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
Update: Regex incorrectly checks newline in comments (fixes #12263) #12407
Conversation
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.
Thanks for working on this!
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, thanks!
The change LGTM, but I think that the commit message and PR title should be At the moment It's probably only a few core rules where that could happen. Example: /* eslint space-before-function-paren:["error", "always"]*/
function foo/* comment
*/(){}
function foo// comment
(){} This can also affect plugins. Might be also good to somehow check will the flag be transpiled for the online demo, because it isn't widely supported in browsers yet. |
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.
Looks good! I agree with @mdjermanovic, this should be an Update as this can add more lint messages in some cases.
Could you please edit the PR title to use the "Update:" prefix? Thanks!
@platinumazure Ok! That make sense! Thanks! |
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.
Looks good. Thanks for changing the commit summary!
We can leave this open another day or two in case more teammates want to review this.
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, thanks!
@platinumazure the commit message is still Fix:
, maybe to leave it as is and change the message while merging the PR?
@mdjermanovic On the GitHub interface, the default/suggested merge commit message is the PR title if there are two or more commits. So if we merge using GitHub, it will suggest the PR title in this case. That's why the "commit-message" status check is currently passing and nothing needs to be done here. (When there is only one commit, that commit's message is suggested/used, so in that case, we would need to edit it on merge.) |
What is the purpose of this pull request? (put an "X" next to item)
[X] Bug fix (#12263)
What changes did you make? (Give an overview)
Fix regex in sourceCode.isSpaceBetweenTokens()
Is there anything you'd like reviewers to focus on?
#12263