Chore: Use messageIds in some of the core rules #9648

j-f1 · 2017-11-21T17:29:44Z

What is the purpose of this pull request? (put an "X" next to item)

[x] Other, please explain:

Extract rule messages for core rules to the rule metadata. See #9165 for the implementation of meta.messages and #6740 for the feature request.

What changes did you make? (Give an overview)

Update core rules matching /^(no-)?[a-e]/ to use messageIds.

This PR also updates the tests for those rules to use messageIds, making the tests easier to read IMHO.

Is there anything you'd like reviewers to focus on?

The IDs used for the messages — Are they awkward? Can you come up with better names?
Did I correctly place blank lines around the messages key?
block-spacing: Should I split it into four messages? (missingBefore, missingAfter, unexpectedBefore, unexpectedAfter)
I’ve changed the interpolation keys in a few places — Are those changes good? Should I change other places? (class-methods-use-this, comma-spacing, consistent-this, dot-notation, no-buffer-constructor, no-caller)

platinumazure · 2018-01-07T02:50:20Z

@j-f1 #9165 is merged 😄 Please rebase at your convenience.

Exception: no-duplicate-imports has more-dynamic rule message generation, which I don’t want to tamper with.

…ssageIds

j-f1 · 2018-01-07T16:55:20Z

Rebased @platinumazure!

ilyavolodin

Very nice! Thanks

platinumazure

I left some suggestions for messageId name changes in a few files.

I think my rationale for what makes some messageIds better than others can be summarized as follows:

If the rule name is really self-evident and has generally one message and one situation for reporting said message, I'm okay with simple names like "unexpected" in most cases (but see last point).
If the rule has to report presence or absence of something depending on configuration, I like "missing" or "unexpected" followed by the thing in question. Bonus points for further adjective phrase modifications (e.g., missingCommaAfterSpace).
We need to be careful about just using a "no" prefix (or not having a prefix) because it's not clear what was found in the code versus what is expected. So I like missing/unexpected a lot better for that reason.
Relational words by themselves (before, after, first, last) are definitely ambiguous. Even if rule configurations use those words, we should try to have descriptive messageIds, such as unexpectedSpaceBeforeComma.
I've been lax about some messages that should be pretty self-explanatory in the context of a rule (e.g., getter in accessor-pairs), so I didn't write notes in those cases. But again, we could improve on those (e.g., getterNotPresent) and make it so users don't need to jump up to the messages object to see what it means. They should be able to guess the message pretty well, as if the message is inline right next to it.

Hope this makes sense, and please speak up if you think this vision is not sensible.

platinumazure · 2018-01-08T20:17:03Z

lib/rules/array-bracket-newline.js

-        ]
+        ],
+        messages: {
+            noOpen: "There should be no linebreak after '['.",


I'm struggling with these messageIds, partially because a name like noOpen might imply either that there is a linebreak but should not be (which is what "noOpen" means here), or that there is no linebreak but should be. I would suggest a name like shouldNotHaveLinebreakOpen or doesNotHaveLinebreakOpen (or slightly less verbose) to make it clear what "no" means here. I like the idea of having messageIds close to what the messages say, so of the two, I would probably favor shouldNotHaveLinebreakOpen.

unexpectedOpeningLinebreak?

platinumazure · 2018-01-08T20:17:29Z

lib/rules/array-bracket-spacing.js

-        ]
+        ],
+        messages: {
+            noAfter: "There should be no space after '{{tokenValue}}'.",


See notes in array-bracket-newline.

unexpectedSpaceAfter?

platinumazure · 2018-01-08T20:18:23Z

lib/rules/array-element-newline.js

+        ],
+
+        messages: {
+            noLineBreak: "There should be no linebreak here.",


See notes in array-bracket-newline.

unexpectedLineBreak?

platinumazure · 2018-01-08T20:21:14Z

lib/rules/capitalized-comments.js

+        ],
+
+        messages: {
+            lowercase: "Comments should not begin with a lowercase character",


I think I'd favor a messageId like commentShouldNotBeLowercase even if it's a bit verbose. This one isn't hugely bothersome to me.

unexpectedLowercaseComment?

platinumazure · 2018-01-08T20:25:17Z

lib/rules/comma-style.js

+        ],
+        messages: {
+            beforeAndAfter: "Bad line breaking before and after ','.",
+            after: "',' should be placed first.",


These names seem unclear. Without context, it's not clear if this is talking about space after comma or comma after space, and which is expected. I would suggest something like commaMustBeFirst.

beforeAndAfter is probably fine but I would suggest commaHasLineBreaksBeforeAndAfter or something less verbose, for clarity.

beforeAndAfter → unexpectedLineBeforeAndAfter and after → expectedCommaFirst?

I ended up using unexpectedLineBeforeAndAfterComma for the beforeAndAfter case.

platinumazure · 2018-01-08T20:26:03Z

lib/rules/computed-property-spacing.js

+        ],
+
+        messages: {
+            before: "There should be no space before '{{tokenValue}}'.",


Unclear what is before/after what (see notes in other files).

unexpectedSpaceBefore and unexpectedSpaceAfter?

platinumazure · 2018-01-08T20:28:33Z

lib/rules/consistent-this.js

+        },
+
+        messages: {
+            aliasIsNotThis: "Designated alias '{{name}}' is not assigned to 'this'.",


I think aliasNotAssignedToThis might be better here.

platinumazure · 2018-01-08T20:30:47Z

lib/rules/curly.js

+        fixable: "code",
+
+        messages: {
+            missing: "Expected { after '{{name}}'{{suffix}}.",


You inherited this one, but... Is there any way we can expand the general {{suffix}} into multiple distinct messages for greater clarity? If that's too much of a pain in the rear, we could handle it separately.

platinumazure · 2018-01-08T20:31:16Z

lib/rules/default-case.js

+        }],
+
+        messages: {
+            missing: "Expected a default case."


This name is probably fine as is, but I think missingDefaultCase or even missingDefault would be that little bit clearer.

platinumazure · 2018-01-08T20:31:49Z

lib/rules/dot-location.js

+        fixable: "code",
+
+        messages: {
+            object: "Expected dot to be on same line as object.",


Maybe expectedDotAfterObject and expectedDotBeforeProperty would be a little clearer?

platinumazure · 2018-01-09T16:11:16Z

@j-f1 All changes you suggested in response to my review sound good to me, thanks!

platinumazure · 2018-01-14T23:34:27Z

Question: Should this be an Update, not a Chore? We're changing the public representation of the affected rules, albeit in a completely backward-compatible manner.

j-f1 · 2018-01-14T23:58:58Z

We're changing the public representation of the affected rules

How?

platinumazure · 2018-01-15T00:06:59Z

@j-f1 In that their metadata will now contain message templates, which could theoretically be used by integrations. As I said, it's backwards-compatible and in fact the behavior won't change one bit, but it does seem like a backwards-compatible API change.

Put another way, we're adding a new feature to the rule metadata, and that should be semver-minor, not semver-patch.

j-f1 changed the title ~~Use messageIds in the core rules~~ Chore: Use messageIds in the core rules Nov 21, 2017

aladdin-add added the chore This change is not user-facing label Nov 23, 2017

j-f1 mentioned this pull request Dec 4, 2017

New: Add context.report({ messageId }) (fixes #6740) #9165

Merged

3 tasks

j-f1 added 2 commits January 7, 2018 06:53

Chore: Update accessor-pairs to use messageIds

3544552

Chore: Convert no-alert report() to new-style

8e25a20

j-f1 force-pushed the use-messageids branch 3 times, most recently from 4d772d8 to df4e84e Compare January 7, 2018 13:20

j-f1 added 8 commits January 7, 2018 08:36

Chore: Update rules starting with 'a' or 'no-a' to use messageIds

ff2f9c9

Chore: Update rules starting with 'b' or 'no-b' to use messageIds

ef410f7

Chore: specify type in no-control-regex.getRegExp

5da3f4c

Chore: Update rules starting with 'c' or 'no-c' to use messageIds

a71366d

Chore: Update rules starting with 'd' or 'no-d' to use messageIds

7b0e8f0

Exception: no-duplicate-imports has more-dynamic rule message generation, which I don’t want to tamper with.

Chore: Update rules starting with 'e' or 'no-e' to use messageIds

6c90475

Chore: Correct the invalid tests in no-empty-function for use with me…

17e2d8a

…ssageIds

Chore: Fix lint errors

4b65957

j-f1 force-pushed the use-messageids branch from df4e84e to 4b65957 Compare January 7, 2018 13:36

ilyavolodin approved these changes Jan 8, 2018

View reviewed changes

platinumazure requested changes Jan 8, 2018

View reviewed changes

Inline functions only used once

e8474b6

Update message names as suggested by @platinumazure

db0f4a1

j-f1 force-pushed the use-messageids branch from 8d57ac3 to 9dd65ca Compare January 15, 2018 13:39

Fix error

916a9a4

j-f1 force-pushed the use-messageids branch from 9dd65ca to 916a9a4 Compare January 15, 2018 13:39

renovate bot mentioned this pull request Mar 23, 2018

Update eslint to 4.19.1 sbardian/DebtTrackerAPI#33

Merged

eslint-deprecated bot locked and limited conversation to collaborators Aug 3, 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 Aug 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: Use messageIds in some of the core rules #9648

Chore: Use messageIds in some of the core rules #9648

j-f1 commented Nov 21, 2017 •

edited

platinumazure commented Jan 7, 2018

j-f1 commented Jan 7, 2018

ilyavolodin left a comment

platinumazure left a comment •

edited

platinumazure Jan 8, 2018

j-f1 Jan 8, 2018

platinumazure Jan 8, 2018

j-f1 Jan 9, 2018

platinumazure Jan 8, 2018

j-f1 Jan 9, 2018

platinumazure Jan 8, 2018

j-f1 Jan 9, 2018

platinumazure Jan 8, 2018

j-f1 Jan 9, 2018

j-f1 Jan 15, 2018

platinumazure Jan 8, 2018

j-f1 Jan 9, 2018

platinumazure Jan 8, 2018

platinumazure Jan 8, 2018

platinumazure Jan 8, 2018

platinumazure Jan 8, 2018

platinumazure commented Jan 9, 2018

platinumazure commented Jan 14, 2018

j-f1 commented Jan 14, 2018

platinumazure commented Jan 15, 2018

Chore: Use messageIds in some of the core rules #9648

Chore: Use messageIds in some of the core rules #9648

Conversation

j-f1 commented Nov 21, 2017 • edited

platinumazure commented Jan 7, 2018

j-f1 commented Jan 7, 2018

ilyavolodin left a comment

Choose a reason for hiding this comment

platinumazure left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

platinumazure commented Jan 9, 2018

platinumazure commented Jan 14, 2018

j-f1 commented Jan 14, 2018

platinumazure commented Jan 15, 2018

j-f1 commented Nov 21, 2017 •

edited

platinumazure left a comment •

edited