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

Consistently add quotes to object keys #838

Closed
vjeux opened this issue Mar 1, 2017 · 19 comments · Fixed by #5934
Closed

Consistently add quotes to object keys #838

vjeux opened this issue Mar 1, 2017 · 19 comments · Fixed by #5934
Assignees
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity.
Milestone

Comments

@vjeux
Copy link
Contributor

vjeux commented Mar 1, 2017

While running through the codebase, prettier happens to conflict with the eslint quote props "consistent-as-needed" ( http://eslint.org/docs/rules/quote-props ):

image

const request = {
  method: 'GET',
  headers: {
    'Accept': 'application/json', // Accept isn't quoted right now
    'Content-Type': 'application/json',
  },
}

Apparently we are now doing "as-needed". The difference is that if one element in an object has quotes, then all of them must.

Given those two examples, it sounds like this is a better way to add quotes, but I wanted to get some feedback before implementing it.

@vjeux vjeux added the status:needs discussion Issues needing discussion and a decision to be made before action can be taken label Mar 1, 2017
@bakkot
Copy link
Collaborator

bakkot commented Mar 1, 2017

(See #90 for the original change.)

I'd support consistent-as-needed, I guess. Though if we're going for consistent, it might be a good time to revisit quoting numeric properties: should ({ '0': 0, a: 1 }) format as ({ '0': 0, 'a': 1 }) or ({ 0: 0, a: 1 })?

@vjeux
Copy link
Contributor Author

vjeux commented May 25, 2017

We're going to leave this the way it is for now. We may want to reconsider in the future but I'll close it for now as it's not actionable.

@ismail-syed
Copy link

Can this be reconsidered/revisited? It seems like the community prefers a consistent-as-needed approach. What are the opposing arguments?

@felixfbecker
Copy link

felixfbecker commented Nov 5, 2017

This is very annoying, we have this all over our codebase for OpenTracing:

span.log({
    event: 'error',
    'error.object': err,
    message: err.message,
    stack: err.stack,
})

And for HTTP header hashes and CSS styles it's very common too.

Everyone I know agrees that quoting the keys consistently makes the code more readable

@oliversturm
Copy link

@felixfbecker FWIW, I don't agree - I think it's perfectly fine just like that.

@lipis lipis added this to the 2.0 milestone Mar 5, 2018
@sambigelow
Copy link

@felixfbecker I agree with @oliversturm. 'consistent-as-needed' is a very readable option.

@felixfbecker
Copy link

@sambigelow @oliversturm was advocating against consistent-as-needed, I am for it

@sambigelow
Copy link

@felixfbecker sorry about that

@alextreppass
Copy link

alextreppass commented Apr 17, 2018

Hi @vjeux can we re-open this ticket? I see it's now present in the 2.0 milestone but this ticket is appearing in the 'Closed' section there.

I'd actually love to be able to get at this prior to 2.0 -- it's a blocker for my current company using Prettier, as we use Closure Compiler in advanced mode and it mangles unquoted object keys.

Would PRs be accepted for this prior to 2.0? Or is this stuck in the 2.0 milestone which is still under discussion (and an unknown time away)?

Thanks

@vjeux
Copy link
Contributor Author

vjeux commented Apr 17, 2018

@alextreppass this issue would not address your use case. In this issue, the goal is to never put quotes around keys except if /one/ key isn't a valid JavaScript identifier, in which case it would put quotes on every single one.

What needs to be done for Closure Compiler is to respect all the quotes as they have meaning. I don't know if we have an issue opened for this. I've initially considered it but was surprised at how few people are actually using closure compiled in advanced mode, I think that you are only the second person to raise this as an issue that they face in practice.

I would not be opposed to adding an option for that use case, since prettier would change the behavior of the program. Could you open another issue so we can discuss there?

@alextreppass
Copy link

alextreppass commented Apr 17, 2018

Thanks for the reply @vjeux - have raised #4327 to specifically talk about consistent (instead of consistent-as-needed) or simply just preserving found quotes

@jennasalau
Copy link

Im so confused.. this support is never coming?

Is the only workaround changing consistent-as-needed to as-needed in my linter?

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Aug 29, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 29, 2018
@j-f1 j-f1 added keep-unlocked and removed locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. labels Sep 3, 2018
@prettier prettier unlocked this conversation Sep 3, 2018
@yuhr
Copy link

yuhr commented Nov 21, 2018

I think there's a reasonable case to support consistent in JS and TS context, rather than consistent-as-needed. It makes some sense to explicitly quote "value-like" property keys. Say we have:

const people = {
  "Alice": {
    age: 20,
    gender: "Female"
  },
  "Bob": {
    age: 24,
    gender: "Male"
  }
}

Then the keys of the object shall be used as an array of values semantically, while the inner keys age and gender shall not. Furthermore, in such case we won't access each property with dot notation like people.Alice, but bracket notation like people["Alice"] is acceptable (note that the latter contains quotes). So the consistent strategy may yield consistency for not only object literals but also property access, I think.

And of course the most expectable use case definitely is just iterating with which we literally can't use dot notation, something like:

for (const name in people) {
  people[name]
}

So there's almost no reason unquote such keys, is there?

@felixfbecker
Copy link

Prettier is about preventing bike shedding over formatting. If you leave it to the developer to decide when to quote and when not, there will be discussions in PRs about when to quote and when not to. I don’t want that to be possible, it’s not worth it.

@dflor003
Copy link

Any update on this? This does not play nicely with the rules defined by the linters such as tslint. Ideally there should be a way to let prettier respect the rules setup by the linter.

@vjeux
Copy link
Contributor Author

vjeux commented Nov 28, 2018

@dflor003 you should take a look at tslint-config-prettier which disables all the tslint rules that conflicts with prettier. https://www.npmjs.com/package/tslint-config-prettier

@dflor003
Copy link

Ah, I'd prefer not to disable those rules. We have a shared tslint configuration that we use across 2 dozen or so repositories and would prefer to keep that as the source of truth.

@mgol
Copy link

mgol commented Feb 15, 2019

I posted a comment in the Prettier 2.0 issue but I see the discussion is still happening in this closed issue, so let me copy it here as well:

When adopting Prettier I try to be as open as possible to its choices & not override too much. Currently, for my own purposes I'm happy to just enable trailingComma: 'all' (to reduce Git diffs) and leave the rest as it is.

Enabling consistent quoting would be a huge issue for me, though, especially with no way to override it. This is because changing one line may trigger changes in many more, especially for large object literals.

Small Git diffs don't just help people visually, they also reduce merge conflicts when multiple people modify code in similar places. While it's fine ESLint has rules allowing such enforcement if someone dislikes the status quo visually, I think an opinionated tool like Prettier should strive to minimize formatting changes needed after making a small change in the code.

The only argument for consistent quoting that I've seen is "it looks better" which is subjective. There are many more objective arguments against it so please don't do it!

@azz azz reopened this Mar 2, 2019
@azz
Copy link
Member

azz commented Mar 2, 2019

We're re-considering this in conjunction with #4327.

@azz azz added type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity. and removed status:needs discussion Issues needing discussion and a decision to be made before action can be taken labels Mar 2, 2019
@azz azz self-assigned this Mar 2, 2019
@azz azz added the status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! label Mar 2, 2019
@azz azz closed this as completed in #5934 Mar 6, 2019
@ikatyang ikatyang modified the milestones: 2.0, 1.17 Apr 12, 2019
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 11, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jul 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity.
Projects
None yet
Development

Successfully merging a pull request may close this issue.