Skip to content
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: Use messageIds in some of the core rules #9648

Merged
merged 15 commits into from Feb 3, 2018

Conversation

@j-f1
Copy link
Contributor

@j-f1 j-f1 commented Nov 21, 2017

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?

@j-f1 j-f1 changed the title Use messageIds in the core rules Chore: Use messageIds in the core rules Nov 21, 2017
@platinumazure
Copy link
Member

@platinumazure platinumazure commented Jan 7, 2018

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

@j-f1 j-f1 force-pushed the use-messageids branch 3 times, most recently from 4d772d8 to df4e84e Jan 7, 2018
@j-f1 j-f1 force-pushed the use-messageids branch from df4e84e to 4b65957 Jan 7, 2018
@j-f1
Copy link
Contributor Author

@j-f1 j-f1 commented Jan 7, 2018

Rebased @platinumazure!

Copy link
Member

@ilyavolodin ilyavolodin left a comment

Very nice! Thanks

Copy link
Member

@platinumazure platinumazure left a comment

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.

]
],
messages: {
noOpen: "There should be no linebreak after '['.",
Copy link
Member

@platinumazure platinumazure Jan 8, 2018

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.

Copy link
Contributor Author

@j-f1 j-f1 Jan 8, 2018

unexpectedOpeningLinebreak?

]
],
messages: {
noAfter: "There should be no space after '{{tokenValue}}'.",
Copy link
Member

@platinumazure platinumazure Jan 8, 2018

See notes in array-bracket-newline.

Copy link
Contributor Author

@j-f1 j-f1 Jan 9, 2018

unexpectedSpaceAfter?

],

messages: {
noLineBreak: "There should be no linebreak here.",
Copy link
Member

@platinumazure platinumazure Jan 8, 2018

See notes in array-bracket-newline.

Copy link
Contributor Author

@j-f1 j-f1 Jan 9, 2018

unexpectedLineBreak?

],

messages: {
lowercase: "Comments should not begin with a lowercase character",
Copy link
Member

@platinumazure platinumazure Jan 8, 2018

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

Copy link
Contributor Author

@j-f1 j-f1 Jan 9, 2018

unexpectedLowercaseComment?

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

@platinumazure platinumazure Jan 8, 2018

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.

Copy link
Contributor Author

@j-f1 j-f1 Jan 9, 2018

beforeAndAfterunexpectedLineBeforeAndAfter and afterexpectedCommaFirst?

Copy link
Contributor Author

@j-f1 j-f1 Jan 15, 2018

I ended up using unexpectedLineBeforeAndAfterComma for the beforeAndAfter case.

],

messages: {
before: "There should be no space before '{{tokenValue}}'.",
Copy link
Member

@platinumazure platinumazure Jan 8, 2018

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

Copy link
Contributor Author

@j-f1 j-f1 Jan 9, 2018

unexpectedSpaceBefore and unexpectedSpaceAfter?

},

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

@platinumazure platinumazure Jan 8, 2018

I think aliasNotAssignedToThis might be better here.

fixable: "code",

messages: {
missing: "Expected { after '{{name}}'{{suffix}}.",
Copy link
Member

@platinumazure platinumazure Jan 8, 2018

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.

}],

messages: {
missing: "Expected a default case."
Copy link
Member

@platinumazure platinumazure Jan 8, 2018

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

fixable: "code",

messages: {
object: "Expected dot to be on same line as object.",
Copy link
Member

@platinumazure platinumazure Jan 8, 2018

Maybe expectedDotAfterObject and expectedDotBeforeProperty would be a little clearer?

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Jan 9, 2018

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

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Jan 14, 2018

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
Copy link
Contributor Author

@j-f1 j-f1 commented Jan 14, 2018

We're changing the public representation of the affected rules

How?

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Jan 15, 2018

@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 j-f1 force-pushed the use-messageids branch from 8d57ac3 to 9dd65ca Jan 15, 2018
@j-f1 j-f1 force-pushed the use-messageids branch from 9dd65ca to 916a9a4 Jan 15, 2018
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Aug 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants