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

Fix: isSpaceBetweenTokens() recognizes spaces in JSXText (fixes #12614) #12616

Merged
merged 5 commits into from Nov 30, 2019

Conversation

@mysticatea
Copy link
Member

mysticatea commented Nov 28, 2019

What is the purpose of this pull request? (put an "X" next to item)

[X] Bug fix: #12614

What changes did you make? (Give an overview)

This PR changes isSpaceBetweenTokens() method to recognize spaces in JSXText tokens in order to fix #12614. The JSXText tokens include whitespaces such as indentation.

Is there anything you'd like reviewers to focus on?

  • Is this direction right?
@mysticatea mysticatea force-pushed the issue12614 branch from e6de965 to 8e73ee3 Nov 28, 2019
lib/source-code/source-code.js Outdated Show resolved Hide resolved
tests/lib/source-code/source-code.js Outdated Show resolved Hide resolved
@ljharb
Copy link
Contributor

ljharb commented Nov 29, 2019

It'd be great if you had the time to verify that eslint-plugin-react's tests pass on master with this change :-)

@mysticatea
Copy link
Member Author

mysticatea commented Nov 29, 2019

I'm sorry, today I failed to get time to follow the review. I will do it tomorrow (JST).

It'd be great if you had the time to verify that eslint-plugin-react's tests pass on master with this change :-)

I confirmed it by copying/pasting this patch to node_modules/eslint and running tests.

mysticatea added 4 commits Nov 30, 2019
@mysticatea mysticatea changed the title Fix: isSpaceBetween should recognize the spaces in JSXText (fixes #12614) Fix: isSpaceBetween() recognizes spaces in JSXText (fixes #12614) Nov 30, 2019
@mysticatea mysticatea changed the title Fix: isSpaceBetween() recognizes spaces in JSXText (fixes #12614) Fix: isSpaceBetweenTokens() recognizes spaces in JSXText (fixes #12614) Nov 30, 2019
@mysticatea
Copy link
Member Author

mysticatea commented Nov 30, 2019

I have updated this PR. Now, this patch doesn't apply to the new isSpaceBetween() method.

Copy link
Member

kaicataldo left a comment

LGTM, thanks!

@kaicataldo kaicataldo added accepted and removed evaluating labels Nov 30, 2019
@kaicataldo kaicataldo merged commit bc435a9 into master Nov 30, 2019
18 checks passed
18 checks passed
Verify Files
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 8.10.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191130.1 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor This change is semver-patch
Details
@kaicataldo kaicataldo deleted the issue12614 branch Nov 30, 2019
ljharb added a commit to ljharb/eslint-plugin-react that referenced this pull request Nov 30, 2019
This reverts commit 71c7d01.

Thanks to v6.7.2 / eslint/eslint#12616 / eslint/eslint#12614
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.