Chore: Rule messages use internal rule message format (fixes #6977) #6989

platinumazure · 2016-08-27T02:14:10Z

What issue does this pull request address?

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:

$ node ./bin/eslint.js --no-eslintrc --rule "prefer-template:error" "./lib/rules/*.js"

mention-bot · 2016-08-27T02:14:11Z

@platinumazure, thanks for your PR! By analyzing the annotation information on this pull request, we identified @vitorbal, @azhang496 and @mysticatea to be potential reviewers

eslintbot · 2016-08-27T02:14:11Z

LGTM

mysticatea · 2016-08-27T03:29:01Z

lib/rules/dot-notation.js

@@ -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.

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
.

mysticatea · 2016-08-27T03:48:53Z

Looks very good to me except some nit-picking.
Thank you!

eslintbot · 2016-08-27T05:02:42Z

LGTM

platinumazure · 2016-08-27T05:03:19Z

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.

mysticatea · 2016-08-27T06:28:51Z

LGTM, thank you!
I'm leaving for other eyes. (this is only 4 hours yet)

platinumazure · 2016-08-28T23:39:59Z

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?

mysticatea · 2016-08-28T23:46:45Z

PRs should remain open for at least 48 hours before merging (72 hours if over the weekend)
#6009

I think a good time is after the patch release on Monday (or we decide the patch release is unnecessary). 😄

platinumazure · 2016-08-28T23:55:29Z

@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

mysticatea · 2016-08-28T23:57:35Z

Oh, I'm sorry. I'm OK 👍
Thank you!

platinumazure · 2016-08-28T23:58:35Z

@mysticatea Thanks. That was my mistake, no need to apologize. Thanks for your feedback!

vitorbal · 2016-08-29T09:32:15Z

lib/rules/valid-jsdoc.js

-                                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.",


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?

vitorbal · 2016-08-29T09:33:33Z

Awesome, nice work @platinumazure! 🎉

platinumazure · 2016-08-29T14:28:15Z

@vitorbal Good call-- I had started with {{ foo }} with spaces because I thought that looked better at first, then I came around. Just forgot to change some of the earlier files I guess. I will fix this in the next 12 hours or so. Thanks for your feedback!

platinumazure · 2016-08-31T14:05:04Z

Apologies, still haven't gotten around to this. I hope to do this tonight (US time).

eslintbot · 2016-09-02T00:47:26Z

LGTM

platinumazure · 2016-09-02T00:49:31Z

@vitorbal Sorry it took so long for me to get this done. Want to take another look?

kaicataldo · 2016-09-02T04:36:01Z

lib/rules/no-dupe-class-members.js

@@ -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?~~

That's my queue to go to bed - my brain thought this was our template syntax ({{}})...

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
.

vitorbal · 2016-09-02T14:31:42Z

@platinumazure done, LGTM!
Thanks for updating all the context.report() calls to the new format as well. Super big effort, good job 👍 👍 !

platinumazure · 2016-09-02T14:33:59Z

Thanks! In fairness I've only done new-style where I was also editing the
message. At some point maybe we should create an internal rule to catch any
remaining old-style uses. I'll try to create an issue for that later.

On Sep 2, 2016 9:31 AM, "Vitor Balocco" notifications@github.com wrote:

@platinumazure https://github.com/platinumazure done, LGTM!
Thanks for updating all the context.report() calls to the new format as
well. Super big effort, good job 👍 👍 !

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6989 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARWeucLcxjEVUIZ1JVfCJditgSAt3Wiks5qmDNPgaJpZM4JulxS
.

vitorbal · 2016-09-02T14:56:21Z

@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!

nzakas · 2016-09-02T17:06:09Z

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

ilyavolodin · 2016-09-02T17:15:16Z

I think it's good to merge as is. If we need any follow-up work, we can do it in separate PR.

platinumazure · 2016-09-02T17:25:35Z

Happy to report the Travis build for the master branch passed with this
change, so no accidental problems were introduced by merging without
rebasing. Thanks for merging this!

On Sep 2, 2016 12:17 PM, "Ilya Volodin" notifications@github.com wrote:

I think it's good to merge as is. If we need any follow-up work, we can do
it in separate PR.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6989 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARWejum8kWIZKqT6udn7MZUyBudvxs5ks5qmFmlgaJpZM4JulxS
.

) (eslint#6989)

jquerybot added the CLA: Valid label Aug 27, 2016

platinumazure mentioned this pull request Aug 27, 2016

Core rule messages should use our template syntax and avoid concatenation #6977

Closed

mysticatea reviewed Aug 27, 2016
View reviewed changes

platinumazure force-pushed the fix-rule-messages branch from 346e1cb to 424446a Compare August 27, 2016 05:02

vitorbal reviewed Aug 29, 2016
View reviewed changes

Chore: Rule messages use internal rule message format (fixes #6977)

d0f4547

platinumazure force-pushed the fix-rule-messages branch from 424446a to d0f4547 Compare September 2, 2016 00:47

kaicataldo reviewed Sep 2, 2016
View reviewed changes

ilyavolodin merged commit 4126f12 into master Sep 2, 2016

ljharb pushed a commit to ljharb/eslint that referenced this pull request Sep 2, 2016

Chore: Rule messages use internal rule message format (fixes eslint#6977

63b431b

) (eslint#6989)

platinumazure deleted the fix-rule-messages branch September 3, 2016 16:50

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: Rule messages use internal rule message format (fixes #6977) #6989

Chore: Rule messages use internal rule message format (fixes #6977) #6989

platinumazure commented Aug 27, 2016 •

edited

mention-bot commented Aug 27, 2016

eslintbot commented Aug 27, 2016

mysticatea Aug 27, 2016

platinumazure Aug 27, 2016

mysticatea commented Aug 27, 2016

eslintbot commented Aug 27, 2016

platinumazure commented Aug 27, 2016

mysticatea commented Aug 27, 2016

platinumazure commented Aug 28, 2016 •

edited

mysticatea commented Aug 28, 2016 •

edited

platinumazure commented Aug 28, 2016

mysticatea commented Aug 28, 2016

platinumazure commented Aug 28, 2016

vitorbal Aug 29, 2016

vitorbal commented Aug 29, 2016

platinumazure commented Aug 29, 2016

platinumazure commented Aug 31, 2016

eslintbot commented Sep 2, 2016

platinumazure commented Sep 2, 2016

kaicataldo Sep 2, 2016 •

edited

platinumazure Sep 2, 2016

vitorbal commented Sep 2, 2016

platinumazure commented Sep 2, 2016

vitorbal commented Sep 2, 2016

nzakas commented Sep 2, 2016

ilyavolodin commented Sep 2, 2016

platinumazure commented Sep 2, 2016

Chore: Rule messages use internal rule message format (fixes #6977) #6989

Chore: Rule messages use internal rule message format (fixes #6977) #6989

Conversation

platinumazure commented Aug 27, 2016 • edited

mention-bot commented Aug 27, 2016

eslintbot commented Aug 27, 2016

mysticatea Aug 27, 2016

Choose a reason for hiding this comment

platinumazure Aug 27, 2016

Choose a reason for hiding this comment

mysticatea commented Aug 27, 2016

eslintbot commented Aug 27, 2016

platinumazure commented Aug 27, 2016

mysticatea commented Aug 27, 2016

platinumazure commented Aug 28, 2016 • edited

mysticatea commented Aug 28, 2016 • edited

platinumazure commented Aug 28, 2016

mysticatea commented Aug 28, 2016

platinumazure commented Aug 28, 2016

vitorbal Aug 29, 2016

Choose a reason for hiding this comment

vitorbal commented Aug 29, 2016

platinumazure commented Aug 29, 2016

platinumazure commented Aug 31, 2016

eslintbot commented Sep 2, 2016

platinumazure commented Sep 2, 2016

kaicataldo Sep 2, 2016 • edited

Choose a reason for hiding this comment

platinumazure Sep 2, 2016

Choose a reason for hiding this comment

vitorbal commented Sep 2, 2016

platinumazure commented Sep 2, 2016

vitorbal commented Sep 2, 2016

nzakas commented Sep 2, 2016

ilyavolodin commented Sep 2, 2016

platinumazure commented Sep 2, 2016

platinumazure commented Aug 27, 2016 •

edited

platinumazure commented Aug 28, 2016 •

edited

mysticatea commented Aug 28, 2016 •

edited

kaicataldo Sep 2, 2016 •

edited