-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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 new schema to no-restricted-syntax (fixes #8298) #8357
Conversation
@vitorbal, thanks for your PR! By analyzing the history of the files in this pull request, we identified @not-an-aardvark, @BYK and @scriptdaemon to be potential reviewers. |
LGTM |
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.
Just one docs question: Should we have an example (or some prose) which indicates that strings and objects can be freely mixed in config?
@platinumazure yeah, sounds like a good idea! I'll add it 👍
…On Wed, Mar 29, 2017 at 8:06 PM Kevin Partington ***@***.***> wrote:
***@***.**** commented on this pull request.
LGTM.
Just one docs question: Should we have an example (or some prose) which
indicates that strings and objects can be freely mixed in config?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8357 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAmNdunK7QjjywfZ-Bee8al4xjuuuTxsks5rqp2mgaJpZM4MtLB7>
.
|
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, just have a few minor nitpicks.
lib/rules/no-restricted-syntax.js
Outdated
context.report({ | ||
node, | ||
message: hasCustomMessage ? selectorOrObject.message : "Using '{{selector}}' is not allowed.", | ||
data: { selector } |
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.
If a custom message contains the substring {{selector}}
, this will cause it to get replaced with the current selector. I doubt this will be a common problem in practice, but do you think it's worth documenting this or updating the logic to avoid it?
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.
Very true! I updated the logic to take this into account and added a test for it. Thanks!
docs/rules/no-restricted-syntax.md
Outdated
@@ -12,7 +12,7 @@ This rule disallows specified (that is, user-defined) syntax. | |||
|
|||
## Options | |||
|
|||
This rule takes a list of strings: | |||
This rule takes a list of strings, where each string is a node type or AST selector: |
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.
Nitpick: Technically, any node type is also an AST selector, so this sentence is redundant. However, I'm fine with keeping it if you think it makes things clearer.
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.
Good point. I changed it to say only "where each string is an AST selector".
LGTM |
d137f14
to
ab745a4
Compare
LGTM |
ab745a4
to
5c38842
Compare
LGTM |
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 changes did you make? (Give an overview)
This PR updates the
no-restricted-syntax
rule to support custom messages, like discussed in #8298.This is a non-breaking enhancement to the rule.
Is there anything you'd like reviewers to focus on?
Any suggestions to make the docs clearer are welcome!