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

Autofixer for newline-after-import. Fixes #686 #696

Merged
merged 1 commit into from
Jun 20, 2017

Conversation

eelyafi
Copy link
Contributor

@eelyafi eelyafi commented Dec 21, 2016

A PR for #686

Will add autofixer for newline-after-import rule.

@eelyafi
Copy link
Contributor Author

eelyafi commented Dec 21, 2016

Keep having this error locally. Can repro on master branch as well. Does anyone know what to do ?

1 failing

  1) package has every rule:
     Uncaught AssertionError: expected { Object (no-unresolved, named, ...) } to have a property 'npm-debug.log'
      at tests/src/package.js:27:14
      at Array.forEach (native)
      at tests/src/package.js:25:15
      at FSReqWrap.oncomplete (fs.js:117:15)

@ljharb
Copy link
Member

ljharb commented Dec 21, 2016

@eelyafi i'm quite sure that's a bug in the test; i'll fix it on master momentarily.

@ljharb
Copy link
Member

ljharb commented Dec 21, 2016

@eelyafi actually, are you using npm test, or manually running mocha tests for the one file?

@eelyafi
Copy link
Contributor Author

eelyafi commented Dec 21, 2016

@ljharb I'm using npm run test

@ljharb
Copy link
Member

ljharb commented Dec 21, 2016

@eelyafi and is your current working directory the root of the project? or is it inside src/rules? (or were you perhaps cd'd into that directory when running an npm command at some point in the past?)

@eelyafi
Copy link
Contributor Author

eelyafi commented Dec 21, 2016

also, after running npm run test there were no linter errors. The errors showed up only after submitting the PR

@ljharb
Copy link
Member

ljharb commented Dec 21, 2016

I was able to reproduce the error by running touch src/rules/npm-debug.log && npm test - but that should only happen otherwise if you were running an npm command not at the root of the repo (all commands should always only be run at the root of any repo). You can rm the npm-debug.log for now to fix it, and I'll push something up to master to make the test more robust.

@eelyafi
Copy link
Contributor Author

eelyafi commented Dec 21, 2016

@ljharb it helped ! thanks :)

@coveralls
Copy link

coveralls commented Dec 21, 2016

Coverage Status

Coverage increased (+0.009%) to 94.869% when pulling 6e426c7 on eelyafi:new_line_fixer into e26e898 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 94.869% when pulling 6e426c7 on eelyafi:new_line_fixer into e26e898 on benmosher:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 94.869% when pulling 6e426c7 on eelyafi:new_line_fixer into e26e898 on benmosher:master.

@coveralls
Copy link

coveralls commented Dec 21, 2016

Coverage Status

Coverage increased (+0.009%) to 94.869% when pulling 6e426c7 on eelyafi:new_line_fixer into e26e898 on benmosher:master.

@eelyafi
Copy link
Contributor Author

eelyafi commented Dec 22, 2016

@ljharb looks like the PR passed all tests but there is no reviewers. How to add one ?

module.exports = {
meta: {
docs: {},
fixable: 'code',

Choose a reason for hiding this comment

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

Similar newline-after-var rule in eslint uses the whitespace option. I haven't found the difference between two options in the docs, but just for consistency I'd suggest to use whitespace.

@coveralls
Copy link

coveralls commented Dec 22, 2016

Coverage Status

Coverage increased (+0.01%) to 94.872% when pulling 2296385 on eelyafi:new_line_fixer into e26e898 on benmosher:master.

@grumd
Copy link

grumd commented Feb 8, 2017

any plans on merging this?

@jfmengels
Copy link
Collaborator

I'm sorry @eelyafi, I forgot about this PR, and I merged #742 before I merged this one, which allows the customization of the number of lines after the imports.
Do you feel like solving the conflicts and fixing the case where that number is customized? Otherwise I'll be happy to take over. Let me know.
And obvisouly, thanks for the PR, it looks great too :)

(sorry again... 😞)

@ljharb ljharb force-pushed the new_line_fixer branch 2 times, most recently from 1e4d07f to 73de417 Compare June 16, 2017 21:39
@ljharb
Copy link
Member

ljharb commented Jun 16, 2017

(This PR is now freshly rebased; thanks @truckingsim for the nudge)

@truckingsim
Copy link

Thanks for letting me know about the rebasing didn't think about that. Appreciate the quick response!

@coveralls
Copy link

coveralls commented Jun 17, 2017

Coverage Status

Coverage increased (+0.008%) to 95.165% when pulling bfdf77a on eelyafi:new_line_fixer into 3f9e4bf on benmosher:master.

@ljharb ljharb requested review from benmosher and jfmengels June 17, 2017 01:27
@ljharb
Copy link
Member

ljharb commented Jun 17, 2017

Let's give this a day or two for extra reviews - after that, I'll plan to merge it.

@ljharb ljharb merged commit d92ef43 into import-js:master Jun 20, 2017
@benmosher
Copy link
Member

published with v2.5.0. thanks for the (months! of) work and attention on this, everyone! 😁

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