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 newlines-between option to order rule #298

Merged
merged 1 commit into from
May 5, 2016
Merged

Add newlines-between option to order rule #298

merged 1 commit into from
May 5, 2016

Conversation

radekbenkel
Copy link
Contributor

Checks existence of new lines between and inside import groups.

Implements #282.

@benmosher
Copy link
Member

LGTM, but @jfmengels should have final say, it's his baby. 😊

@benmosher
Copy link
Member

Oh, actually, it would be great if you could add a note to the change log. ### Added section makes sense to me.

previousImport.node, 'There should be one empty line between import groups'
)
} else if (currentImport.rank === previousImport.rank
&& getLineDifference(currentImport, previousImport) === 2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

=== 2 --> >= 2

@jfmengels
Copy link
Collaborator

This looks pretty good @singles, thanks a lot for this!
Just some comments on the documentation mostly.

it's his baby

Man, you have no idea... I still haven't had the heart to deprecate eslint-plugin-import-order. It's my most popular repo, and growing in downloads every day... Will do it soon... soon...

@benmosher
Copy link
Member

I can imagine. But every download here is your download too, now! 😁 I think GitHub itself may be responsible for the recent uptick in downloads, so that's pretty cool.

@jfmengels
Copy link
Collaborator

I think https://github.com/dustinspecker/awesome-eslint is partially responsible too. Even more so probably.

@radekbenkel
Copy link
Contributor Author

radekbenkel commented May 4, 2016

OK, new changes:

  1. Added info to the changelog
  2. Fixed Add newlines-between option to order rule #298 (comment)
  3. Changed option values from true|false to always|never
  4. Updated docs, less words, more concise

Thx @benmosher @jfmengels for feedback!

@@ -7,6 +7,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
### Added
- [`newline-after-import`], new rule. ([#245], thanks [@singles])
- Added an `optionalDependencies` option to [`no-extraneous-dependencies`] to allow/forbid optional dependencies ([#266], thanks [@jfmengels]).
- Added `newlines-between` option to `order` rule ([#298], thanks [@singles])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you put brackts around order so that there is a link there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@jfmengels
Copy link
Collaborator

Sorry for the nitpicking, looks great otherwise @singles!

Checks existence of new lines between and inside import groups.
@radekbenkel
Copy link
Contributor Author

Sorry for the nitpicking

No problem :)

@jfmengels
Copy link
Collaborator

Looks pretty great to me :D

@benmosher benmosher merged commit f75d0af into import-js:master May 5, 2016
@jfmengels
Copy link
Collaborator

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.

3 participants