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

Disallow quotes around object literal property names that are not strictly required (quote-props) #791

Closed
tunnckoCore opened this issue Feb 21, 2017 · 14 comments

Comments

@tunnckoCore
Copy link

tunnckoCore commented Feb 21, 2017

Digging more and more in ESLint rules, i found some good ones which enforces good style and more readable code. This issue is for quote-props rule - fixable.

Suggested configuration.

{
  "quote-props": ["error", "consistent-as-needed", { "keywords": true }]
}

Invalid

// NOT OK, should not have quotes
// autofix: quotes will be removed
var x = {
  'prop': 1,
  'bar': 'foo'
}

// NOT OK, should be consistent
// autofix: quotes will be removed
var x = {
  'prop': 1,
  bar: 'foo'
}

// NOT OK, should be consistent
// autofix: will set quotes for `prop`
var x = {
  prop: 1,
  'foo-bar': 'foo'
}

// NOT OK, all props should have quotes
// because `while` is keyword
var x = {
  while: 1,
  volatile: 'foo'
}

Valid

var x = {
  prop: 1,
  bar: 'foo'
}

var x = {
  prop: 1,
  bar: 'foo'
}

var x = {
  'prop': 1,
  'foo-bar': 'foo'
}

var x = {
  'while': 1,
  'volatile': 'foo'
}
@dcousens
Copy link
Member

NACK. as-needed is the only approach I typically take.

var x = {
  'while': 2,
  prop: 1,
  'foo-bar': 'foo'
}

@tunnckoCore
Copy link
Author

Me too, but it is messy for the eyes and we have autofix feature, so absolutely no problem.

@dcousens
Copy link
Member

dcousens commented Feb 22, 2017

@tunnckoCore not true.

If I have an object with 100 properties that has no quotes, then suddenly introduce foo-bar, I'm not going to then re-quote each of those properties.
Even with --fix.

What a mess that would be for any diff too.

@tunnckoCore
Copy link
Author

Kinda didn't thout for that case. But in any way

If I have an object with 100 properties that has no quotes

is some exotic case

@dcousens
Copy link
Member

is some exotic case

If we're using objects this much, even an object of 3-5 properties would feel like 100 properties.
It's a pain.

NACK.

@timoxley
Copy link
Contributor

timoxley commented Feb 22, 2017

The intention of this rule is good, however I agree with @dcousens. Requoting unrelated keys due to adding a single property that needs quoting is not great.

it is messy for the eyes and we have autofix feature, so absolutely no problem.

I think we should avoid any rules while lean heavily on --fix for day-to-day use, a typical standard user shouldn't need know anything about standard outside of the warnings it produces. IMO we should only rely on --fix as an escape-hatch for upgrades to standard.

@tunnckoCore
Copy link
Author

tunnckoCore commented Feb 22, 2017

So, maybe as-needed then?

I believe everyone should by default call standard with --fix flag. It just not make sense to see such errors while they can be fixed. Autofix implementations aren't dangerous, or i still didn't see such.

I even would suggest fix to be enabled by default. And standard officially to recommend using it at least.

@ghost
Copy link

ghost commented Mar 22, 2017

👍 on catching reserved keywords, maybe. A big 👎 on forcing too much coding style (even if fixable). Standard provides coding bliss because it steps out of the way and allows Zen coding w/o interruption. Add too much opinion and you lose that zen. Related reading: http://ben.balter.com/2016/07/21/removing-a-feature-is-a-feature/

@feross
Copy link
Member

feross commented Apr 4, 2017

@tunnckoCore I added a recommendation to use --fix at the very top of the readme. Thanks for the suggestion! See here: https://github.com/feross/standard#javascript-style-guide-with-linter--automatic-code-fixer

@timoxley I think we should avoid any rules while lean heavily on --fix for day-to-day use, a typical standard user shouldn't need know anything about standard outside of the warnings it produces

This is correct. As cool as --fix is, it's really important to me that I can write code that passes standard easily (from memory and without relying on --fix because something is too painful to do manually).

@feross
Copy link
Member

feross commented Apr 4, 2017

The ecosystem impact of this is 7.5%.

# tests 238
# pass  220
# fail  18

However, this is mitigated by the fact that the rule is fixable. Most of the failing cases are places where folks were consistently using quoted props, even though they're not needed. Like this:

var yargKeys = {
  'boolean': { transform: null, error: null },
  'count': { transform: null, error: null },
  'string': { transform: null, error: null },
  'array': { transform: null, error: null },
  'required': { transform: null, error: null },
  'default:': { transform: null, error: null },
  'choices:': { transform: null, error: null },
}

We could try enabling this in a future beta version, so I'll leave this open for now.

@feross feross added this to the standard v11 milestone Apr 4, 2017
@feross feross changed the title Proposal: Consistent quote props Disallow quotes around object literal property names that are not strictly required (quote-props) Apr 4, 2017
@jaridmargolin
Copy link

Prettier implements as-needed and I believe it greatly affects readability:

screen shot 2018-06-30 at 12 14 59 pm

I prefer consistent/consistent-as-needed, but understand the argument against:

If I have an object with 100 properties that has no quotes, then suddenly introduce foo-bar, I'm not going to then re-quote each of those properties.

I would prefer standard does not take a stance on this. I philosophically believe in what standardjs is doing in order avoid bikeshedding (or at least move it to a single repo 😂), but this rule greatly affects my ability to quickly scan/read code in my editor. The implementation of as-needed would most likely drive me away from standardjs, which would be very sad 😢

@jaridmargolin
Copy link

One additional note: Prettier is considering "Consistently add quotes to object keys" for v2.0. I would assume this change is due to overwhelming community feedback (although my assumption is probably based on my bias):

With the following comment coming from @vjeux who appears to want to adjust this rule regardless of a v2.0 release:

For consistent key quotes, I think we should just do it regardless, I’m not sure why we haven’t already.

prettier/prettier#3503

@feross feross modified the milestones: standard v12, standard v13 Aug 28, 2018
@mightyiam
Copy link
Member

as-needed is my personal preference.

@feross
Copy link
Member

feross commented Jul 5, 2019

I think we should go ahead with the following rule in standard 13.

"quote-props": ["error", "as-needed"]

Ecosystem breakage is only 4%. --fix can be used to automatically fix the issue. Furthermore, the rule is easy to remember: add quotes only where you are required to.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

No branches or pull requests

6 participants