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.

Sign up for GitHub

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

Jump to bottom

Fix orders newline-between for multiline imports #329

Closed
wants to merge 1 commit into from
Closed

Fix orders newline-between for multiline imports #329

wants to merge 1 commit into from

Conversation

singles
Copy link
Contributor

@singles singles commented May 10, 2016 •
edited

Partly-fixes #313.

jfmengels
jfmengels reviewed May 10, 2016
View reviewed changes
tests/src/rules/order.js
['builtin', 'index'],
['sibling'],
['parent', 'external'],
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not that it matters all that much, but you could simplify the tests by removing the groups option, as you don't need it.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant in all the tests you added and where it's not needed. I think you only removed it from the last one?

@jfmengels
Copy link
Collaborator

LGTM.

You say this partially fixes #313, what part of it remains? What I suggested adding tests for in #313 (comment)?

@singles
Copy link
Contributor Author

singles commented May 10, 2016

@jfmengels Yes, this is pretty tricky, but looks like I'm not far away from finishing that. Also case with require in nightwatch config isn't solved yet.

@jfmengels
Copy link
Collaborator

jfmengels commented May 10, 2016 •
edited

Not sure what you meant with nightwatch config.

@singles
Copy link
Contributor Author

singles commented May 11, 2016

@jfmengels That part with requires of selenium and chromedrvier - it comes from our nightwatch.js file. Sorry for not being clear enough.

@singles
Copy link
Contributor Author

singles commented May 13, 2016

I will create new PR which solves all issues mentioned in #313, closing that one.

@singles singles closed this May 13, 2016
@singles singles deleted the bugfix-order-newlines-between-multiline-import branch May 13, 2016 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

order's newlines-between doesn't work with multiline import statements
2 participants
@singles @jfmengels