Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Disable rules when fixing on save #90

Merged
merged 5 commits into from
Feb 19, 2018
Merged

Disable rules when fixing on save #90

merged 5 commits into from
Feb 19, 2018

Conversation

pvdlg
Copy link
Contributor

@pvdlg pvdlg commented Nov 28, 2017

Fix #89

Add a new config that allows to define rules that will not be automatically fixed when saving.
Set by default with capitalized-comments and no-only-test.

Errors will still be fixed when running the command XO:Fix.

The test currently fails because an issue with deep-assign in XO. When passing the fix as a function, deep-assign replaces it with {}.
Passing the option {fix: () => {...}} through deep-assign results in {fix: {}}.

This is fixed by 4f4de13, but this commit is not released yet. As soon a new version of XO is released the tests will work.

@pvdlg pvdlg force-pushed the disable-rules-on-save branch from 139a49f to b62cbda Compare November 28, 2017 00:42
@sindresorhus
Copy link
Owner

This is fixed by 4f4de13, but this commit is not released yet. As soon a new version of XO is released the tests will work.

Can you bump XO here?

@pvdlg pvdlg force-pushed the disable-rules-on-save branch from 1a693e2 to ad73769 Compare February 14, 2018 16:04
@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 14, 2018

Done !

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 14, 2018 •
edited
Loading

It seems there is an issue on the Travis build: Xlib: extension "RANDR" missing on display ":99".

I tried pretty everything I could think of to solve that issue, unsuccessfully...

Maybe an environment variable mising due to the orga change?

@pvdlg pvdlg force-pushed the disable-rules-on-save branch 16 times, most recently from 9644c52 to ad73769 Compare February 15, 2018 07:06
@Arcanemagus
Copy link

Arcanemagus commented Feb 15, 2018 •
edited
Loading

It seems there is an issue on the Travis build: Xlib: extension "RANDR" missing on display ":99".

This can safely be ignored.

FYI, the officially recommended CI scripts are located in https://github.com/atom/ci, although nobody has bothered to fix it there because it doesn't actually change anything.

If you were curious, to fix this you need to add +extension RANDR to the Xvfb arguments. (Did that while creating a Docker image for Atom CI builds.)

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 15, 2018

Thanks for the info! Really helpful.

So I have to figure out why apm test fails on Travis but works locally...

@Arcanemagus
Copy link

Took a look at the build, generally when apm test has no output like that, it means that Atom encountered an error before the specs could start. Usually this is due to encountering syntax it doesn't understand, but it could be anything.

You can probably get this output to show by changing apm test to atom --enable-electron-logging --test spec.js. (I think that's the right syntax for a single file?)

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 15, 2018

Thanks!
For info I tried to use the official script from https://github.com/atom/ci. apm works but atom doesn't. After installing ~/atom/usr/bin/atom -v doesn't return anything.

@pvdlg pvdlg force-pushed the disable-rules-on-save branch 2 times, most recently from aba0091 to 9facd94 Compare February 16, 2018 02:55
@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 16, 2018

It failed :( https://travis-ci.org/xojs/atom-linter-xo/builds/342209740#L1859

Unfortunately I can't run atom --enable-electron-logging --test spec.js in the CI because the atom bin is not found.

@pvdlg pvdlg force-pushed the disable-rules-on-save branch from 6a0935f to 87cf5b1 Compare February 16, 2018 06:34
@Arcanemagus
Copy link

Fascinating, I can't get it to fail locally under any of the environments I have set up. The next step I would take would be to essentially stick the important parts of build-package.sh directly in the build script so I could insert the debugging version instead, or alternatively fork that script, insert it, and switch the build here to use the forked version temporarily.

I can work on that tomorrow if you don't figure it out before hand.

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 16, 2018

That's a good idea! I'll try that, but it's getting very late for me so it will have to wait until tomorrow as well. Any help is much appreciated!

@pvdlg pvdlg force-pushed the disable-rules-on-save branch from b404d06 to f20d201 Compare February 16, 2018 06:51
@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 16, 2018

language: generic in .travis.yml seems to fix the Atom spec.
I guess it's only because by default the generic image happen to come with a Node version compatible with Atom.
If you run with a Node image that specifies version 8 or 9 then you get:

[6797:0216/064237.292221:INFO:CONSOLE(109)] "Error: The module '/home/travis/build/xojs/atom-linter-xo/node_modules/pathwatcher/build/Release/pathwatcher.node'
was compiled against a different Node.js version using
NODE_MODULE_VERSION 59. This version of Node.js requires
NODE_MODULE_VERSION 53. Please try re-compiling or re-installing
the module (for instance, using `npm rebuild` or`npm install`).

I imagine the day Travis decide to update their default Node version on their generic image that will happen as well.

@pvdlg pvdlg force-pushed the disable-rules-on-save branch from f20d201 to 886d7ef Compare February 16, 2018 16:24
@Arcanemagus
Copy link

Actually, the issue with language: node is that unless you override it Travis automatically "helps" you by running npm install, which happens before the script step. In this case that is building the wrong version of the native modules.

If you want to use that image you just need to skip the install step.

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 16, 2018

Thanks for the info. So I guess I can use a Node image and if I override the install step to run what I have now it should work.

@pvdlg pvdlg force-pushed the disable-rules-on-save branch from 886d7ef to 7aaa558 Compare February 16, 2018 18:20
@Arcanemagus
Copy link

Arcanemagus commented Feb 16, 2018 •
edited
Loading

Looks like that did it!

Or not, it's back to 0 tests 😕.

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 16, 2018

Yep I noticed as well... It's really weird...

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 16, 2018

ah my bad, I renamed the test spec.js by mistake

@pvdlg pvdlg force-pushed the disable-rules-on-save branch 2 times, most recently from 120231d to 474c15a Compare February 16, 2018 18:41
@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 16, 2018

Seems good now, the test are running now https://travis-ci.org/xojs/atom-linter-xo/builds/342474730#L540

@Arcanemagus
Copy link

Looks like a .DS_Store file snuck in there. 😆

@pvdlg pvdlg force-pushed the disable-rules-on-save branch from 474c15a to 53b94ba Compare February 16, 2018 19:02
@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 16, 2018

Looks like a .DS_Store file snuck in there. 😆

Oops. It's fixed now.

sindresorhus
sindresorhus reviewed Feb 17, 2018
View reviewed changes
lib/fix.js Outdated
return text => {
return lint(editor)(text, {fix: true})
return (text, exclude) => {
const fix = exclude ? report => exclude.indexOf(report.ruleId) === -1 : true;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indexOf => includes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

sindresorhus
sindresorhus reviewed Feb 19, 2018
View reviewed changes
.travis.yml
install:
- curl -s -O https://raw.githubusercontent.com/atom/ci/master/build-package.sh
- chmod u+x build-package.sh
- ./build-package.sh
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wooorm FYI

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sindresorhus Hmm? Do we need a change in atom-linter-xo?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I mean, we had to stop using wooorm/atom-travis because it was no longer working.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, I missed the context. That’s a pity! Thanks for letting me know. I’ll investigate it when I need to use it again as well 👍

@sindresorhus sindresorhus merged commit 114c664 into sindresorhus:master Feb 19, 2018
@sindresorhus
Copy link
Owner

This looks great. Thanks for helping us out @Arcanemagus :)

@pvdlg pvdlg deleted the disable-rules-on-save branch February 19, 2018 06:39
@wooorm wooorm mentioned this pull request Feb 19, 2018
Closed
bramkok added a commit to bramkok/eslint-config-ibood that referenced this pull request Mar 3, 2018
@bramkok
Override capitalization of comments
4b1900c 
When `xo` is used to fix on each save of a file commented out code lines
get capitalized. See  sindresorhus/atom-linter-xo#90 for
more information.
@pvdlg pvdlg mentioned this pull request Mar 10, 2018
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wooorm wooorm wooorm left review comments

@sindresorhus sindresorhus sindresorhus left review comments

@Arcanemagus Arcanemagus Arcanemagus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow to disable some rules when fixing on save
4 participants
@pvdlg @sindresorhus @Arcanemagus @wooorm