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
Conversation
1b7c6bd
to
509a92e
Compare
509a92e
to
95cefe7
Compare
e8f642d
to
68ea9fd
Compare
68ea9fd
to
a3fd4fc
Compare
On further reflection, I think that it would make sense to rename this method to 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. |
77d2067
to
c48c806
Compare
I think a simple TSC decision would be sufficient, personally. |
TSC Summary
TSC Question
EDIT: Please see #12519 for this proposal. This PR is now just the bug fix. |
I'm actually going to split this out into two separate PRs - one for the bug fix and one for the deprecation proposal. |
c48c806
to
a3fd4fc
Compare
New PR added here. |
7c126d3
to
41a959a
Compare
1e3ee5f
to
e901a71
Compare
e901a71
to
8a7042f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
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):
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?