Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add autofixer for order rule #908

Merged
merged 4 commits into from
Mar 28, 2018
Merged

Add autofixer for order rule #908

merged 4 commits into from
Mar 28, 2018

Conversation

tihonove
Copy link
Contributor

@tihonove tihonove commented Aug 2, 2017

I really want this feature :-)
Fixes #711. Fixes #547.

@coveralls
Copy link

coveralls commented Aug 2, 2017

Coverage Status

Coverage increased (+0.05%) to 96.02% when pulling 31def14 on tihonove:master into dd28130 on benmosher:master.

@coveralls
Copy link

coveralls commented Aug 2, 2017

Coverage Status

Coverage decreased (-0.006%) to 95.959% when pulling c25bdcb on tihonove:master into dd28130 on benmosher:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 95.959% when pulling c25bdcb on tihonove:master into dd28130 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 95.959% when pulling c25bdcb on tihonove:master into dd28130 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 95.959% when pulling c25bdcb on tihonove:master into dd28130 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.131% when pulling 2209e83 on tihonove:master into dd28130 on benmosher:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.131% when pulling 2209e83 on tihonove:master into dd28130 on benmosher:master.

@coveralls
Copy link

coveralls commented Aug 2, 2017

Coverage Status

Coverage increased (+0.2%) to 96.131% when pulling 2209e83 on tihonove:master into dd28130 on benmosher:master.

@tihonove
Copy link
Contributor Author

tihonove commented Aug 2, 2017

Hi, i dont have ideas how to fix appveyor build. Could you give me a hint?
Another point: travis builds on MacOS seems to unstable. Here is failed build and after fix coverage success build

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

What happens when there's non-require non-import statements between the mis-ordered items? I wouldn't expect this rule to enforce "all imports/requires are grouped together/at the top"

@tihonove
Copy link
Contributor Author

tihonove commented Aug 3, 2017

@ljharb, currently autofixer just fix errors reported by original rule. Code

var async = require('async');
var a = 1;
var fs = require('fs');

transforms to

var fs = require('fs');
var async = require('async');
var a = 1;

due to rule requires 'fs' to occurs before 'async'.
In my opinion fixer should fix errors reported by rule.
Should i modify rule to avoid this behavior?

@ljharb
Copy link
Member

ljharb commented Aug 3, 2017

What about:

var b = require('b');
require('a')(b);

?

In other words, I think this rule is only safely fixable in a subset of the circumstances that the rule warns on - eslint core's policy (which I think everyone should follow) is that autofixes must always be "safe", and not change the meaning of the code, even if that means not autofixing some things.

@tihonove
Copy link
Contributor Author

tihonove commented Aug 3, 2017

🙈 Currently autofixer brokes the code above. Im going to update autofixer to avoid unsafe fixes.

@ljharb
Copy link
Member

ljharb commented Aug 3, 2017

Specifically: I think that to start, autofixing can only be said to be safe only within a chunk of requires and/or imports, where the requires are purely assignments - and any code that's not a variable-assigned-require or an import-with-a-binding has to be considered a boundary across which autofixing can't cross.

For example, side-effecting requires/imports (like just import 'foo'; or require('foo');, eg polyfills/shims/etc), are likely in place intentionally, which means that every require/import that happens before it must remain before it, and everything after it must remain after it.

@tihonove
Copy link
Contributor Author

Hi. Currently reorder fixes is very safe and only strict var-assignement-require and imports can be reordered and autofixes would not applied to the following code

var foo = require('./foo').bar.bar.bar;
var fs = require('fs');

Im not sure about this. Maybe such reorderings should be allowed

@coveralls
Copy link

coveralls commented Aug 14, 2017

Coverage Status

Coverage increased (+0.2%) to 96.117% when pulling 316d818 on tihonove:master into dd28130 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.118% when pulling 82a66c4 on tihonove:master into dd28130 on benmosher:master.

1 similar comment
@coveralls
Copy link

coveralls commented Aug 14, 2017

Coverage Status

Coverage increased (+0.2%) to 96.118% when pulling 82a66c4 on tihonove:master into dd28130 on benmosher:master.

@ljharb
Copy link
Member

ljharb commented Aug 14, 2017

@tihonove using fs would make it safe; but if you imported something with side effects it wouldn't be :-/

@tihonove
Copy link
Contributor Author

Ok, are there anything i can do to complete pr?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks, this is very thorough.

We'll need to get the tests passing before it can be merged tho.

test({
code: `
var async = require('async');
var fs = require('fs');` + ' ' + `
Copy link
Member

Choose a reason for hiding this comment

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

you could do:

var fs = require('fs');${' '}

without having to use string concatenation

message: '`fs` import should occur before import of `async`',
}],
})),
// reorder cant cross variable assignemet
Copy link
Member

Choose a reason for hiding this comment

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

it's not really the variable assignment that's an issue here; it's what's on the RHS - in other words, anything that's depending on a global, or on built-in prototypes, or on any imported value.

Probably safest tho to stick with "variable assignment"

@coveralls
Copy link

coveralls commented Aug 16, 2017

Coverage Status

Coverage increased (+0.2%) to 96.145% when pulling 9af8223 on tihonove:master into dd28130 on benmosher:master.

@tihonove
Copy link
Contributor Author

We'll need to get the tests passing before it can be merged tho.

I dont have ideas how to fix appveyor build. Could you give me a hint? Seems it fails after all test successfully passed

@ljharb
Copy link
Member

ljharb commented Aug 16, 2017

I believe the appveyor build might be broken on master; @benmosher, can you confirm?

@lukeapage
Copy link
Contributor

Would be great to see this merged..

@lukeapage
Copy link
Contributor

@benmosher any chance of merging and releasing this?

@lukeapage
Copy link
Contributor

@tihonove - master build is now passing again
@benmosher - any chance you can take a look at merging this?

@coveralls
Copy link

coveralls commented Oct 26, 2017

Coverage Status

Coverage increased (+0.1%) to 96.364% when pulling 41c4540 on tihonove:master into 1eac942 on benmosher:master.

@mrm007
Copy link

mrm007 commented Dec 8, 2017

It sure would be great to have this merged 🙏

@lukeapage
Copy link
Contributor

@ljharb I see you have merge rights now. Given you approved this and Ben does not seem to be about, would you consider merging it? Thanks.

@ljharb
Copy link
Member

ljharb commented Jan 15, 2018

@lukeapage i've had them this whole time; I don't think an autofixer - especially one as risky as an ordering autofixer - should be merged with only one collaborator having reviewed it.

@MoOx
Copy link
Contributor

MoOx commented Mar 27, 2018

@benmosher anything we can do to make this review easy? It could be really nice to get this in a release :)

Copy link
Contributor

@MoOx MoOx left a comment

Choose a reason for hiding this comment

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

I just tried this on a pretty big codebase, worked like a charm without any issues!

CHANGELOG.md Outdated
@@ -9,6 +9,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
## [2.8.0] - 2017-10-18
### Added
- [`exports-last`] rule ([#620] + [#632], thanks [@k15a])
- Autofixer for [`order`] rule ([#711], thanks [@tihonove])
Copy link
Contributor

Choose a reason for hiding this comment

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

Might need to be adjusted

Copy link
Member

@benmosher benmosher left a comment

Choose a reason for hiding this comment

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

not super familiar with fixers or this rule in particular, but tests look good and @MoOx's anecdote about running it on a big project is a big deal. thanks for this!

@benmosher
Copy link
Member

benmosher commented Mar 28, 2018

will merge once linux builds pass 👍(I finally fixed the windows build)

@MoOx
Copy link
Contributor

MoOx commented Mar 28, 2018

macOS build seems to be struggling. Are those really necessary btw (as this lib doesn't involve any "macos" specific code?)

@benmosher
Copy link
Member

@MoOx I agree that it shouldn't matter but the tests are there for the times when it mysteriously does. 😅 LGTM now, though 👍

@ljharb
Copy link
Member

ljharb commented Apr 19, 2018

See #1086 - this PR caused a performance regression. It’d be great if anyone interested in keeping this autofixer in the plugin could look into that issue.

@ajhyndman
Copy link

You're my hero, @tihonove! I just needed to apply an import/order rule to 395 files; this saved me a full day of hair pulling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

8 participants