-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Chore: Rule messages use internal rule message format (fixes #6977) #6989
Conversation
@platinumazure, thanks for your PR! By analyzing the annotation information on this pull request, we identified @vitorbal, @azhang496 and @mysticatea to be potential reviewers |
LGTM |
@@ -51,18 +51,30 @@ module.exports = { | |||
node.computed && | |||
node.property.type === "Literal" && | |||
validIdentifier.test(node.property.value) && | |||
(allowKeywords || keywords.indexOf("" + node.property.value) === -1) | |||
(allowKeywords || keywords.indexOf(`${node.property.value}`) === -1) |
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.
In this case, personally, I prefer String(node.property.value)
instead.
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.
Oh, good call. I'll see what other opportunities there might be.
On Aug 26, 2016 10:29 PM, "Toru Nagashima" notifications@github.com wrote:
In lib/rules/dot-notation.js
#6989 (comment):@@ -51,18 +51,30 @@ module.exports = {
node.computed &&
node.property.type === "Literal" &&
validIdentifier.test(node.property.value) &&
(allowKeywords || keywords.indexOf("" + node.property.value) === -1)
(allowKeywords || keywords.indexOf(`${node.property.value}`) === -1)
In this case, personally, I prefer String(node.property.value) instead.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/eslint/eslint/pull/6989/files/346e1cbdf22426535f3bb747525afaeab3301d3a#r76509316,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARWepeZX8ihdc4Ee_fYLMURIgoWMktTks5qj67-gaJpZM4JulxS
.
Looks very good to me except some nit-picking. |
346e1cb
to
424446a
Compare
LGTM |
Thanks @mysticatea for reviewing. I've force-pushed with your suggested changes (along with a few similar issues I found via grep). Please let me know if I've missed anything. |
LGTM, thank you! |
I'm thinking I'll probably merge this in 24 hours or so (EDIT: After a patch release decision), unless someone objects (or unless someone reviews the PR and identifies a problem with it). @mysticatea Does that sound okay to you? |
I think a good time is after the patch release on Monday (or we decide the patch release is unnecessary). 😄 |
@mysticatea Apologies, I meant after a patch release but forgot to write that down. (I had assumed a decision on a patch release would happen within 24 hours). Editing my previous post to reflect this. Thanks |
Oh, I'm sorry. I'm OK 👍 |
@mysticatea Thanks. That was my mistake, no need to apologize. Thanks for your feedback! |
context.report(jsdocNode, "Unexpected @" + tag.title + " tag; function has no return statement."); | ||
context.report({ | ||
node: jsdocNode, | ||
message: "Unexpected @{{ title }} tag; function has no return statement.", |
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 noticed most other messages are doing {{foo}}
(no whitespaces inside the brackets). I guess it's more of a nitpick, but maybe we do that here too to keep it consistent?
Awesome, nice work @platinumazure! 🎉 |
@vitorbal Good call-- I had started with |
Apologies, still haven't gotten around to this. I hope to do this tonight (US time). |
424446a
to
d0f4547
Compare
LGTM |
@vitorbal Sorry it took so long for me to get this done. Want to take another look? |
@@ -34,7 +34,7 @@ module.exports = { | |||
*/ | |||
function getState(name, isStatic) { | |||
const stateMap = stack[stack.length - 1]; | |||
const key = "$" + name; // to avoid "__proto__". | |||
const key = `$${name}`; // to avoid "__proto__". |
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 like there are two $
here - is that intentional?
That's my queue to go to bed - my brain thought this was our template syntax ({{}}
)...
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.
Yes, look at the removed line- it has a $ prefix. (That said, if I need to
escape it and I'm doing it wrong, please let me know!)
On Sep 1, 2016 11:36 PM, "Kai Cataldo" notifications@github.com wrote:
In lib/rules/no-dupe-class-members.js
#6989 (comment):@@ -34,7 +34,7 @@ module.exports = {
*/
function getState(name, isStatic) {
const stateMap = stack[stack.length - 1];
const key = "$" + name; // to avoid "**proto**".
const key = `$${name}`; // to avoid "**proto**".
Looks like there are two $ here - is that intentional?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/eslint/eslint/pull/6989/files/d0f454703eb527a515a55bbe2d2680ebc72d5382#r77292146,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARWer5RnEHKtqsU5YsYevq_aQAVn5rVks5ql6eygaJpZM4JulxS
.
@platinumazure done, LGTM! |
Thanks! In fairness I've only done new-style where I was also editing the On Sep 2, 2016 9:31 AM, "Vitor Balocco" notifications@github.com wrote:
|
@platinumazure if we plan on deprecating the old style we could change it all on one swoop? Not sure if worth an additional custom rule. But let's discuss in the new issue! |
Can this be merged? It's a huge PR, I'd like to avoid the pain of merge conflicts by getting this in sooner rather than later |
I think it's good to merge as is. If we need any follow-up work, we can do it in separate PR. |
Happy to report the Travis build for the master branch passed with this On Sep 2, 2016 12:17 PM, "Ilya Volodin" notifications@github.com wrote:
|
What issue does this pull request address?
Issue #6977.
What changes did you make? (Give an overview)
Using the prefer-template rule, I fixed all violations in lib/rules. Rule messages were transformed into rule substitution format, and everything else turned into a template string.
Oh, except for no-regex-spaces due to issue #6988.
Is there anything you'd like reviewers to focus on?
I had to use a template string for no-regex-spaces due to issue #6988. Hope that's okay.
Please feel free to pull the branch down locally and run a command like the following in order to check that I didn't miss anything: