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 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-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Oct 24, 2019
@kaicataldo kaicataldo added bug ESLint is working incorrectly enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels 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
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.

@platinumazure
Copy link
Member

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

@kaicataldo kaicataldo added core Relates to ESLint's core APIs and features tsc agenda This issue will be discussed by ESLint's TSC at the next meeting labels Oct 27, 2019
@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

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

@kaicataldo
Copy link
Member Author

kaicataldo commented Nov 1, 2019

New PR added here.

@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion tsc agenda This issue will be discussed by ESLint's TSC at the next meeting labels Nov 1, 2019
@kaicataldo kaicataldo force-pushed the fix-is-space-between-tokens branch 2 times, most recently from 1e3ee5f to e901a71 Compare November 7, 2019 02:12
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@kaicataldo kaicataldo merged commit 9e29e18 into master Nov 7, 2019
@kaicataldo kaicataldo deleted the fix-is-space-between-tokens branch November 7, 2019 22:32
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators May 7, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label May 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants