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

Added tests for jsx-curly-spacing that deal with comments in children. #1414

Merged
merged 3 commits into from Feb 19, 2018
Merged

Added tests for jsx-curly-spacing that deal with comments in children. #1414

merged 3 commits into from Feb 19, 2018

Conversation

s-h-a-d-o-w
Copy link
Contributor

See also:
#648 (comment)

@oyeanuj
Copy link

oyeanuj commented Nov 4, 2017

@ljharb Does this seem good to merge? I'm also running into this problem.

@ljharb ljharb force-pushed the jsx-curly-spacing-children-tests branch from 8715216 to 36a2008 Compare December 20, 2017 00:42
@ljharb
Copy link
Member

ljharb commented Dec 20, 2017

I've rebased this, and one test is still failing.

@ljharb
Copy link
Member

ljharb commented Dec 20, 2017

Notably, the one failing test is just about failing to remove a space from before a }; no comments are removed.

@s-h-a-d-o-w s-h-a-d-o-w mentioned this pull request Feb 15, 2018
Closed
@s-h-a-d-o-w @ljharb
[Fix] jsx-curly-spacing problem in new tests, fixes jsx-eslint#1414
e4de360 
Before this, spaces before/after braces when there should be none resulted in two overlapping fixes for the full brace content.
And so they required two passes. But the tester does only one pass, so only one white space was fixed and the test failed.

By including comments in the range calculation for the fix, the range is now properly limited, making it possible to have start and end fixes in one pass.
s-h-a-d-o-w added a commit to s-h-a-d-o-w/eslint-plugin-react that referenced this pull request Feb 16, 2018
@s-h-a-d-o-w
[Fix] jsx-curly-spacing problem in new tests, fixes jsx-eslint#1414
9219e52 
Before this, spaces before/after braces when there should be none resulted in two overlapping fixes for the full brace content.
And so they required two passes. But the tester does only one pass, so only one white space was fixed and the test failed.

By including comments in the range calculation for the fix, the range is now properly limited, making it possible to have start and end fixes in one pass.
@s-h-a-d-o-w
Copy link
Contributor Author

Sweet, looks like this is good to go (see my commit message for details about what the problem was).

@ljharb ljharb force-pushed the jsx-curly-spacing-children-tests branch from 9219e52 to e4de360 Compare February 19, 2018 22:35
ljharb
ljharb approved these changes Feb 19, 2018
View reviewed changes
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.

Looks great, thanks!

@ljharb ljharb merged commit e4de360 into jsx-eslint:master Feb 19, 2018
@renovate renovate bot mentioned this pull request Mar 22, 2018
Merged
@ljharb ljharb mentioned this pull request Apr 10, 2018
Closed
This was referenced Sep 22, 2018
Closed
Closed
Closed
Open
Merged
Open
Open
Closed
Open
Open
Open
This was referenced Sep 23, 2018
Open
Open
Open
Open
Merged
Open
Open
Closed
Open
Open
Open
Merged
Open
This was referenced Oct 25, 2018
Merged
Open
This was referenced Nov 7, 2018
Merged
Open
This was referenced Nov 23, 2018
Open
Merged
@renovate renovate bot mentioned this pull request Dec 5, 2018
Closed
1 task
@mesosphere-web mesosphere-web mentioned this pull request Dec 19, 2018
Closed
1 task
This was referenced Dec 21, 2018
Open
Merged
Merged
Closed
This was referenced Jan 2, 2019
Merged
Closed
Open
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
bug help wanted
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@s-h-a-d-o-w @oyeanuj @ljharb