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

tools: .eslintrc.js messages "default" typo style #22868

Closed
wants to merge 3 commits into from

Conversation

lovinglyy
Copy link
Contributor

make all messages in .eslintrc.js follow a in common style

Checklist

* all methods and syntax keywords between backticks
* written numbers
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Sep 14, 2018
* moving /* eslint-disable max-len */
@lpinca
Copy link
Member

lpinca commented Sep 16, 2018

It seems a common style is already shared by all messages, what is the advantage of adding backticks?

@lovinglyy
Copy link
Contributor Author

Actually, a single error message has it: https://github.com/nodejs/node/blob/master/.eslintrc.js#L163 so I thought that all should have it, or none, and if any new message is added it will probably follow the pattern of the others, I'm aware that it's a small change and it can not be that relevant(no problem in closing the pr).
I think that the effectiveness in readability for backticks is more noticeable in keywords, that at a first glance can be part of the text, like in Use new keyword when throwing an Error..

.eslintrc.js Outdated
},
{
selector: 'ThrowStatement > CallExpression[callee.name=/Error$/]',
message: 'Use new keyword when throwing an Error.',
message: 'Use `new` keyword when throwing an Error.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Add backticks around Error as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, it looks better in the sentence now

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not usually a fan of using markdown conventions (such as backticks around code) in places that aren't markdown, but I agree this improves readability, especially for the `new` keyword one, so 👍 from me.

@danbev
Copy link
Contributor

danbev commented Sep 18, 2018

@addaleax
Copy link
Member

addaleax commented Sep 24, 2018

Landed in 4e09fc3 🎉

@addaleax addaleax closed this Sep 24, 2018
addaleax pushed a commit that referenced this pull request Sep 24, 2018
"Default" typo pattern for .eslintrc.js messages

* all methods and syntax keywords between backticks
* written numbers
* moving /* eslint-disable max-len */
* Error object backtick

PR-URL: #22868
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Sep 25, 2018
"Default" typo pattern for .eslintrc.js messages

* all methods and syntax keywords between backticks
* written numbers
* moving /* eslint-disable max-len */
* Error object backtick

PR-URL: #22868
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants