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: sourceCode#isSpaceBetweenTokens() checks non-adjacent tokens #12491

Merged
merged 4 commits into from Nov 7, 2019

Conversation

@kaicataldo
Copy link
Member

kaicataldo commented Oct 24, 2019

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

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

I noticed that sourceCode#isSpaceBetweenTokens() assumes that the given arguments are tokens and that they are adjacent, but there is nothing stopping a consumer from using non-adjacent tokens as well as AST nodes (example here).

I think it would make sense to only allow tokens that are adjacent as arguments, however, that would be a breaking change. This PR updates the logic so that it will check for any whitespace between tokens (including comments) between two given nodes or tokens. The only difference in behavior is the following two tests (two tokens with a string containing spaces between them is currently incorrectly being flagged as containing whitespace):

// Both of the following should return false but currently return true.

a/* */+' /**/ '/* */+c

a/* */+` /*
*/ `/* */+c

The rest of the tests cover the current behavior that already exists.

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

Does this seem like the correct solution to the rest of the team?

@eslint eslint bot added the triage label Oct 24, 2019
@kaicataldo kaicataldo force-pushed the fix-is-space-between-tokens branch from 1b7c6bd to 509a92e Oct 24, 2019
@kaicataldo kaicataldo force-pushed the fix-is-space-between-tokens branch from 509a92e to 95cefe7 Oct 24, 2019
lib/source-code/source-code.js Outdated Show resolved Hide resolved
tests/lib/source-code/source-code.js Outdated Show resolved Hide resolved
@kaicataldo kaicataldo force-pushed the fix-is-space-between-tokens branch from e8f642d to 68ea9fd Oct 24, 2019
@kaicataldo kaicataldo force-pushed the fix-is-space-between-tokens branch from 68ea9fd to a3fd4fc Oct 26, 2019
@kaicataldo
Copy link
Member Author

kaicataldo commented Oct 26, 2019

On further reflection, I think that it would make sense to rename this method to isSpaceBetween() and leave the ability for it to check between nodes and tokens, as that's a useful behavior that is most likely being used by custom rules (since we use it in at least one of our core rules). We could deprecate isSpaceBetweenTokens() and leave it as an alias that calls through to isSpaceBetween() for the sake of backwards compatibility.

Would it make sense to make an RFC for this or just flag this for TSC discussion? Just to reiterate, the only change in behavior are the examples described above. Aside from that bug fix, this is adding tests and documentation for existing behavior.

@kaicataldo kaicataldo force-pushed the fix-is-space-between-tokens branch from 77d2067 to c48c806 Oct 26, 2019
@platinumazure
Copy link
Member

platinumazure commented Oct 27, 2019

I think a simple TSC decision would be sufficient, personally.

@kaicataldo
Copy link
Member Author

kaicataldo commented Oct 29, 2019

TSC Summary

While fixing a bug in in sourceCode#isSpaceBetweenTokens(), I noticed that the current documentation is incorrect, also making the name of the method not quite correct.

TSC Question

In addition to the bug fix/documentation corrections, I'd like to propose we deprecate sourceCode#isSpaceBetweenTokens() to in favor of sourceCode#isSpaceBetween(). This more accurately describes the behavior, as the method currently will check between nodes as well as tokens, and better matches the names of other sourceCode methods that check between nodes or tokens.

This PR currently deprecates sourceCode#isSpaceBetweenTokens() in a backwards compatible way, having it call through to the new method.

EDIT: Please see #12519 for this proposal. This PR is now just the bug fix.

@kaicataldo
Copy link
Member Author

kaicataldo commented Nov 1, 2019

I'm actually going to split this out into two separate PRs - one for the bug fix and one for the deprecation proposal.

@kaicataldo kaicataldo force-pushed the fix-is-space-between-tokens branch from c48c806 to a3fd4fc Nov 1, 2019
@kaicataldo
Copy link
Member Author

kaicataldo commented Nov 1, 2019

New PR added here.

@kaicataldo kaicataldo force-pushed the fix-is-space-between-tokens branch from 7c126d3 to 41a959a Nov 7, 2019
@kaicataldo kaicataldo force-pushed the fix-is-space-between-tokens branch 2 times, most recently from 1e3ee5f to e901a71 Nov 7, 2019
@kaicataldo kaicataldo force-pushed the fix-is-space-between-tokens branch from e901a71 to 8a7042f Nov 7, 2019
Copy link
Member

mysticatea left a comment

LGTM, thank you!

@kaicataldo kaicataldo merged commit 9e29e18 into master Nov 7, 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 #20191107.9 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 No patch release is pending
Details
@kaicataldo kaicataldo deleted the fix-is-space-between-tokens branch Nov 7, 2019
@eslint eslint bot locked and limited conversation to collaborators May 7, 2020
@eslint eslint bot added the archived due to age label May 7, 2020
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.

None yet

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