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: Add consistent option to computed-property-spacing #12406
Conversation
* @returns {string} Text between specified tokens. | ||
* @public | ||
*/ | ||
getTextBetweenTokens(first, second) { |
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.
I'm not opposed to adding this to our public API, but I think the other two added methods should be implemented in rules or, if shared, in ast-utils
.
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.
These methods may be useful for other rules too.
I'm not sure about moving these methods into ast-utils
.
I've extracted code for methods from isSpaceBetweenTokens
and made the code reusable.
If considering this idea, then isSpaceBetweenTokens
can also be moved into ast-utils
.
* @public | ||
*/ | ||
static removeInlineComments(text) { | ||
return text.replace(/\/\*.*?\*\//gu, ""); |
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.
I don't think this it's a good idea to add this to the public API. This check only works when given the text between two adjacent tokens. If allowed any arbitrary text, a string containing this pattern could be mutated.
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.
As stated in name and comment this method is intended to remove inline comments i.e. /* */
comments inside a line. So it is quite generic and can be helpful (at least in this module).
But I can replace the method by private function.
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.
I understand. However, this would also mutate the following text:
const constructComment = `/*${insertCommentText()}*/`;
becomes
const constructComment = ``;
I actually think the code this is being lifted from in isSpaceBetweenTokens
is also buggy, and there is a PR open to fix it.
Closed in favor of #12560. |
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What rule do you want to change?
computed-property-spacing
Does this change cause the rule to produce more or fewer warnings?
The rule will produce fewer warnings for some code.
How will the change be implemented? (New option, new default behavior, etc.)?
New options.
Please provide some example code that this change will affect:
What does the rule currently do for this code?
Reports a warning.
What will the rule do after it's changed?
Will not report warnings with
"consistent"
option.What changes did you make? (Give an overview)
I've added
"consistent"
andmaxSpaces
options to make the rule more flexible.Is there anything you'd like reviewers to focus on?
I've added new methods (
getTextBetweenTokens
andgetCleanTextBetweenTokens
) inSourceCode
class and have changed implementation ofisSpaceBetweenTokens
method.