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

[import/order] Fix alphabetize bug with newlines-between #1562

Merged
merged 1 commit into from Dec 9, 2019
Merged

[import/order] Fix alphabetize bug with newlines-between #1562

merged 1 commit into from Dec 9, 2019

Conversation

AamuLumi
Copy link
Contributor

@AamuLumi AamuLumi commented Dec 9, 2019

Hello!

I tested the new alphabetize rule, and when used with newlines-between: "always", the plugin mark as invalid any lines which have not newline around.

For example :

import Constants from '@constants';
import Logger from '@server/tools/logger';

These lines are OK with this plugin configuration (1) :

'import/order': [
	'error',
	{
		groups: [['builtin', 'external'], 'internal', 'parent', 'sibling'],
		'newlines-between': 'always',
	},
],

but are invalid with this plugin configuration (2) :

'import/order': [
	'error',
	{
		alphabetize: { order: 'asc' },
		groups: [['builtin', 'external'], 'internal', 'parent', 'sibling'],
		'newlines-between': 'always',
	},
],

And the configuration (2) mark these lines as valid :

import Constants from '@constants';

import Logger from '@server/tools/logger';

I checked the alphabetize PR, and I saw this comment which is exactly the bug I have.
The next comment gives a solution to the problem.

So I just implemented the solution, and I also add the associated test.

Fixes #1561.

@AamuLumi AamuLumi changed the title Fix alphabetize bug with newlines-between [import/order] Fix alphabetize bug with newlines-between Dec 9, 2019
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 96.325% when pulling 9a3bafc on AamuLumi:master into 977da57 on benmosher:master.

@coveralls
Copy link

coveralls commented Dec 9, 2019

Coverage Status

Coverage decreased (-0.2%) to 96.127% when pulling 614e55f on AamuLumi:master into 2d669b1 on benmosher:master.

@idpaterson
Copy link

Linking to #1561 in case anyone else is working on a PR for the same issue.

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!

@ljharb ljharb merged commit 614e55f into import-js:master Dec 9, 2019
@caseywebdev
Copy link

Any chance we can get a version 2.19.2 with this in it? I love the new alphabetize option but I also want the newlines-between option 😬

moeriki added a commit to moeriki/eslint-noise that referenced this pull request Dec 19, 2019
@caseywebdev
Copy link

@ljharb what do you think about cutting a 2.19.2 with this bugfix?

@ljharb
Copy link
Member

ljharb commented Dec 20, 2019

It remains on my list of things to do; there's still something i'm the middle of fixing on this repo first.

@caseywebdev
Copy link

Sounds good, thanks

@ljharb
Copy link
Member

ljharb commented Jan 11, 2020

v2.20.0 is released.

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.

import/order: alphabetize doesn't work correctly with group & newlines-between
5 participants