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

Use single quotes in no-undef context.report message #4845

Closed
lencioni opened this Issue Jan 2, 2016 · 12 comments

Comments

Projects
None yet
5 participants
@lencioni
Copy link
Contributor

lencioni commented Jan 2, 2016

While working on a different project that consumes the output of ESLint
and some plugins, we noticed that the quote styles emitted by the
no-undef rule was different than the quote styles emitted by some of the
rules in our other plugins
. This inconsistency led to a small bug in
our plugin, so I wanted to smooth things out a bit for future
developers.

To make things more consistent, my first inclination was to change the
plugin to match ESLint's no-undef quote style, but upon investigation it
seems that most of the core ESLint rules use single quotes instead of
double quotes, so I think it makes sense to change this one to be more
consistent.

There may be other inconsistencies that are worth resolving, but #4840
fixes the main pain point for me.

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Jan 2, 2016

@lencioni Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Jan 2, 2016

@lencioni

This comment has been minimized.

Copy link
Contributor Author

lencioni commented Jan 2, 2016

The version of ESLint you are using (run eslint -v)

v1.10.3

What you did (the source code and ESLint configuration)

.eslintrc.js:

module.exports = {
  "env": {
    "browser": true
  },
  "rules": {
    "no-undef": 2
  }
};

index.js:

console.log(thisIsUndef);
eslint index.js

The actual ESLint output complete with numbers

~/eslint-example ❯❯❯ eslint index.js

/Users/joe/eslint-example/index.js
  1:13  error  "thisIsUndef" is not defined  no-undef

✖ 1 problem (1 error, 0 warnings)

What you expected to happen instead

~/eslint-example ❯❯❯ eslint index.js

/Users/joe/eslint-example/index.js
  1:13  error  'thisIsUndef' is not defined  no-undef

✖ 1 problem (1 error, 0 warnings)

lencioni added a commit to lencioni/eslint that referenced this issue Jan 2, 2016

Fix: Use single quotes in no-undef context.report message
While working on a different project that consumes the output of ESLint
and some plugins, we noticed that the quote styles emitted by the
no-undef rule was different than the quote styles emitted by some of the
rules in our other plugins [0]. This inconsistency led to a small bug in
our plugin, so I wanted to smooth things out a bit for future
developers.

To make things more consistent, my first inclination was to change the
plugin to match ESLint's no-undef quote style, but upon investigation it
seems that most of the core ESLint rules use single quotes instead of
double quotes, so I decided to change this one to be more consistent.

There may be other inconsistencies that are worth resolving, but this
commit fixes the main pain point for me.

Fixes eslint#4845

[0]: Galooshi/import-js#85 (comment)

lencioni added a commit to lencioni/eslint that referenced this issue Jan 2, 2016

Fix: Use single quotes in no-undef context.report message
While working on a different project that consumes the output of ESLint
and some plugins, we noticed that the quote styles emitted by the
no-undef rule was different than the quote styles emitted by some of the
rules in our other plugins [0]. This inconsistency led to a small bug in
our plugin, so I wanted to smooth things out a bit for future
developers.

To make things more consistent, my first inclination was to change the
plugin to match ESLint's no-undef quote style, but upon investigation it
seems that most of the core ESLint rules use single quotes instead of
double quotes, so I decided to change this one to be more consistent.

There may be other inconsistencies that are worth resolving, but this
commit fixes the main pain point for me.

[0]: Galooshi/import-js#85 (comment)

(fixes eslint#4845)

lencioni added a commit to lencioni/eslint that referenced this issue Jan 2, 2016

Fix: Use single quotes in no-undef messages (fixes eslint#4845)
While working on a different project that consumes the output of ESLint
and some plugins, we noticed that the quote styles emitted by the
no-undef rule was different than the quote styles emitted by some of the
rules in our other plugins [0]. This inconsistency led to a small bug in
our plugin, so I wanted to smooth things out a bit for future
developers.

To make things more consistent, my first inclination was to change the
plugin to match ESLint's no-undef quote style, but upon investigation it
seems that most of the core ESLint rules use single quotes instead of
double quotes, so I decided to change this one to be more consistent.

There may be other inconsistencies that are worth resolving, but this
commit fixes the main pain point for me.

[0]: Galooshi/import-js#85 (comment)
@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Jan 3, 2016

While it would be good to have consistency here, your motivation worries me. The pretty formatter is meant to be consumed by humans. If a program you are writing is consuming the output of eslint, you should be using a formatter that is creating output that is meant to be consumed in this way.

@lencioni

This comment has been minimized.

Copy link
Contributor Author

lencioni commented Jan 3, 2016

Sorry, I should have specified. We are actually using the compact formatter. I'm looking over the formatters that are available and thinking that json or unix would perhaps be a little better for us, but wouldn't resolve this problem for us.

lencioni added a commit to lencioni/import-js that referenced this issue Jan 3, 2016

Use eslint unix format instead of compact format
Although these formatters produce pretty similar output, it seems that
the unix formatter is made to be machine readable whereas the compact
one may not have been designed with that goal in mind. This should help
this portion of ImportJS be more stable as things change.

There are other machine readable formatters available that I considered,
such as json, but since unix was so simple I decided to go with it.

Thanks to @michaelficarra for the nudge [0].

[0]: eslint/eslint#4845 (comment)
@lencioni

This comment has been minimized.

Copy link
Contributor Author

lencioni commented Jan 3, 2016

I just noticed that the other rule we are consuming, no-unused-vars, also uses double quotes instead of single quotes, so I'll add another commit to this PR to change that.

lencioni added a commit to lencioni/eslint that referenced this issue Jan 3, 2016

Fix: Use single quotes in no-undef messages (fixes eslint#4845)
While working on a different project that consumes the output of ESLint
and some plugins, we noticed that the quote styles emitted by the
no-undef rule was different than the quote styles emitted by some of the
rules in our other plugins [0]. This inconsistency led to a small bug in
our plugin, so I wanted to smooth things out a bit for future
developers.

To make things more consistent, my first inclination was to change the
plugin to match ESLint's no-undef quote style, but upon investigation it
seems that most of the core ESLint rules use single quotes instead of
double quotes, so I decided to change this one to be more consistent.

There may be other inconsistencies that are worth resolving, but this
commit fixes the main pain point for me.

[0]: Galooshi/import-js#85 (comment)

lencioni added a commit to lencioni/eslint that referenced this issue Jan 3, 2016

Fix: Use single quotes in no-unused-vars messages (fixes eslint#4845)
While working on a different project that consumes the output of ESLint
and some plugins, we noticed that the quote styles emitted by the
no-unused-vars rule was different than the quote styles emitted by some
of the rules in our other plugins [0]. This inconsistency led to a small
bug in our plugin, so I wanted to smooth things out a bit for future
developers.

To make things more consistent, my first inclination was to change the
plugin to match ESLint's no-unused-vars quote style, but upon
investigation it seems that most of the core ESLint rules use single
quotes instead of double quotes, so I decided to change this one to be
more consistent.

There may be other inconsistencies that are worth resolving, but this
commit fixes the main pain point for me.

[0]: Galooshi/import-js#85 (comment)

lencioni added a commit to lencioni/eslint that referenced this issue Jan 3, 2016

Fix: Use single quotes in no-undef and no-unused-vars (fixes eslint#4845
)

While working on a different project that consumes the output of ESLint
and some plugins, we noticed that the quote styles emitted by the
no-undef and no-unused-vars rule was different than the quote styles
emitted by some of the rules in our other plugins [0]. This
inconsistency led to a small bug in our plugin, so I wanted to smooth
things out a bit for future developers.

To make things more consistent, my first inclination was to change the
plugin to match ESLint's no-undef and no-unused-vars quote style, but
upon investigation it seems that most of the core ESLint rules use
single quotes instead of double quotes, so I decided to change these
rules to be more consistent.

There may be other inconsistencies that are worth resolving, but this
commit fixes the main pain point for me.

[0]: Galooshi/import-js#85 (comment)

lencioni added a commit to lencioni/eslint that referenced this issue Jan 3, 2016

Fix: Use single quotes in no-undef and no-unused-vars (fixes eslint#4845
)

While working on a different project that consumes the output of ESLint
and some plugins, we noticed that the quote styles emitted by the
no-undef and no-unused-vars rule was different than the quote styles
emitted by some of the rules in our other plugins [0]. This
inconsistency led to a small bug in our plugin, so I wanted to smooth
things out a bit for future developers.

To make things more consistent, my first inclination was to change the
plugin to match ESLint's no-undef and no-unused-vars quote style, but
upon investigation it seems that most of the core ESLint rules use
single quotes instead of double quotes, so I decided to change these
rules to be more consistent.

There may be other inconsistencies that are worth resolving, but this
commit fixes the main pain point for me.

[0]: Galooshi/import-js#85 (comment)
@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Jan 4, 2016

I think ESLint rules are using double quotes consistently.
Though it's not catching in string, we are using quotes: [2, "double"] in our code.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 4, 2016

@mysticatea he means inside strings, such as "Missing 'foo'". This is not covered by the rule.

Before we go any further on this, I'd like to see a breakdown of rules using double vs. single quotes to make sure single is actually the predominant one, otherwise we will just keep converting things back and forth. @lencioni can you do that?

@lencioni

This comment has been minimized.

Copy link
Contributor Author

lencioni commented Jan 4, 2016

Sure, I'd be happy to! I'll probably dig into this tomorrow.

@lencioni

This comment has been minimized.

Copy link
Contributor Author

lencioni commented Jan 5, 2016

I ran ag context.report.*{{ -- lib/rules after my commit that converts 2 rules to singles and got the following output. Although this isn't a perfect method, it seems close enough for our needs.

lib/rules/camelcase.js:38:        context.report(node, "Identifier '{{name}}' is not in camel case.", { name: node.name });
lib/rules/complexity.js:50:            context.report(node, "Function '{{name}}' has a complexity of {{complexity}}.", { name: name, complexity: complexity });
lib/rules/id-match.js:60:        context.report(node, "Identifier '{{name}}' does not match the pattern '{{pattern}}'.", {
lib/rules/jsx-quotes.js:53:                context.report(attributeValue, "Unexpected usage of {{description}}.", setting);
lib/rules/max-depth.js:50:            context.report(node, "Blocks are nested too deeply ({{depth}}).",
lib/rules/max-nested-callbacks.js:42:            context.report(node, "Too many nested callbacks ({{num}}). Maximum allowed is {{max}}.", opts);
lib/rules/max-params.js:26:            context.report(node, "This function has too many parameters ({{count}}). Maximum allowed is {{max}}.", {
lib/rules/no-alert.js:30:    context.report(node, "Unexpected {{name}}.", { name: identifierName });
lib/rules/no-arrow-condition.js:64:            context.report(node, "Arrow function `=>` used inside {{statementType}} instead of comparison operator.", {statementType: node.type});
lib/rules/no-bitwise.js:31:        context.report(node, "Unexpected use of '{{operator}}'.", { operator: node.operator });
lib/rules/no-caller.js:21:                context.report(node, "Avoid arguments.{{property}}.", { property: propertyName });
lib/rules/no-catch-shadow.js:50:                context.report(node, "Value of '{{name}}' may be overwritten in IE 8 and earlier.",
lib/rules/no-cond-assign.js:108:            context.report(ancestor, "Unexpected assignment within {{type}}.", {
lib/rules/no-dupe-class-members.js:76:                context.report(node, "Duplicate name \"{{name}}\".", {name: name});
lib/rules/no-dupe-keys.js:36:                        context.report(node, property.loc.start, "Duplicate key '{{key}}'.", { key: keyName });
lib/rules/no-empty-label.js:20:                context.report(node, "Unexpected label \"{{l}}\"", {l: node.label.name});
lib/rules/no-inner-declarations.js:50:            context.report(node, "Move {{type}} declaration to {{body}} root.",
lib/rules/no-obj-calls.js:20:                    context.report(node, "'{{name}}' is not a function.", { name: name });
lib/rules/no-octal-escape.js:29:                    context.report(node, "Don't use octal: '\\{{octalDigit}}'. Use '\\u....' instead.",
lib/rules/no-new-wrappers.js:19:                context.report(node, "Do not use {{fn}} as a constructor.", { fn: node.callee.name });
lib/rules/no-restricted-imports.js:28:                    context.report(node, "'{{importName}}' import is restricted from being used.", {
lib/rules/no-restricted-modules.js:65:                    context.report(node, "'{{moduleName}}' module is restricted from being used.", {
lib/rules/no-restricted-syntax.js:21:        context.report(node, "Using \"{{type}}\" is not allowed.", node);
lib/rules/no-undef-init.js:23:                context.report(node, "It's not necessary to initialize '{{name}}' to undefined.", { name: name });
lib/rules/prefer-reflect.js:47:        context.report(node, "Avoid using {{existing}}, instead use {{substitute}}", {
lib/rules/quote-props.js:146:                context.report(node, "Properties should be quoted as `{{property}}` is a reserved word.", {property: key.name});
lib/rules/quote-props.js:152:                context.report(node, "Inconsistently quoted property `{{key}}` found.", {
lib/rules/valid-jsdoc.js:127:                            context.report(jsdocNode, "Missing JSDoc parameter type for '{{name}}'.", { name: tag.name });
lib/rules/valid-jsdoc.js:131:                            context.report(jsdocNode, "Missing JSDoc parameter description for '{{name}}'.", { name: tag.name });
lib/rules/valid-jsdoc.js:135:                            context.report(jsdocNode, "Duplicate JSDoc parameter '{{name}}'.", { name: tag.name });
lib/rules/valid-jsdoc.js:174:                    context.report(jsdocNode, "Use @{{name}} instead.", { name: prefer[tag.title] });
lib/rules/valid-jsdoc.js:196:                            context.report(jsdocNode, "Expected JSDoc for '{{name}}' but found '{{jsdocName}}'.", {
lib/rules/valid-jsdoc.js:201:                            context.report(jsdocNode, "Missing JSDoc for parameter '{{name}}'.", {

It looks like a bunch of rules don't use quotes, 2 use backticks (quote-props, no-arrow-condition), 3 use double quotes (no-dupe-class-members, no-empty-label, no-restricted-syntax), and 12 use single quotes (camelcase, complexity, id-match, no-bitwise, no-catch-shadow, no-dupe-keys, no-obj-calls, no-octal-escape, no-restricted-imports, no-restricted-modules, no-undef-init, valid-jsdoc).

I'd be happy to submit changes to make more things consistent if you are open to that.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 5, 2016

Nice work! Yeah, I'd love a PR to make things consistent. Keep in mind that some messages use string concatenation, so there are likely more outliers in the codebase.

@nzakas nzakas added enhancement rule accepted and removed triage labels Jan 5, 2016

@lencioni

This comment has been minimized.

Copy link
Contributor Author

lencioni commented Jan 5, 2016

Cool. I'll update my PR shortly.

@lencioni

This comment has been minimized.

Copy link
Contributor Author

lencioni commented Jan 5, 2016

@nzakas You were right, I did find more outliers. I have pushed an updated commit that changes all of the ones that I found by searching for context.report and verifying them manually.

@nzakas nzakas closed this in 58715e9 Jan 6, 2016

yannickcr added a commit to yannickcr/eslint-plugin-react that referenced this issue Jan 11, 2016

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

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.