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

Change default of singleQuote to true #7466

Closed
wants to merge 15 commits into from
Closed

Change default of singleQuote to true #7466

wants to merge 15 commits into from

Conversation

karlhorky
Copy link
Contributor

@karlhorky karlhorky commented Jan 29, 2020

Change default of singleQuote to true.

Closes #4102

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Why?

Single quotes are more common. This will make Prettier more zero-config for the majority.

Additionally:

Votes from Prettier Collaborators / Members:

👍 @j-f1
👍 @lipis
👍 @evilebottnawi
👍 @sosukesuzuki

👎 @lydell
👎 @Shinigami92

Try the playground for this PR

@karlhorky
Copy link
Contributor Author

karlhorky commented Jan 29, 2020

Uh, this is my first PR that changes the defaults... I'm not sure that I did everything correctly. It seems like there aren't a lot of changes to the snapshots, which I was expecting when I ran:

yarn test -u
yarn lint --fix

Can someone give me some guidance? cc @lipis @evilebottnawi

I modeled most of the things off of of @kachkaev's #7430.

@karlhorky
Copy link
Contributor Author

Ok I think I figured it out - I changed the wrong options file.

@lipis
Copy link
Member

lipis commented Jan 29, 2020

You should target the next branch.. and it's better to checkout to that branch first..

@lipis lipis changed the base branch from master to next January 29, 2020 11:32
@lipis
Copy link
Member

lipis commented Jan 29, 2020

try

git pull https://github.com/prettier/prettier.git next

Merge it and push back to your branch..

@lipis lipis marked this pull request as ready for review January 29, 2020 11:47
@lipis
Copy link
Member

lipis commented Jan 29, 2020

Otherwise branch out from next do your changes and create a new PR :)

@karlhorky
Copy link
Contributor Author

karlhorky commented Jan 29, 2020

Yep, just cherry picked my changes to a new branch (force pushed to same name, so we can keep this PR), branched from next.

Just updating the snapshots now... Looks like there are some tests that use hardcoded strings instead of snapshots, and those will have to be updated too.

docs/rationale.md Outdated Show resolved Hide resolved
@thorn0
Copy link
Member

thorn0 commented Jan 29, 2020

500+ files changed. This PR is going to be really difficult to review.

@karlhorky
Copy link
Contributor Author

karlhorky commented Jan 29, 2020

I think a lot of it can be quickly checked (maybe even partially automated), since the vast majority is in the snapshots.

Automation options:

  • remove all diffs of snapshots that are only changing the quotes in the same positions (this will probably remove 90%+ of diffs)
  • @fisker's idea of: making a temp snapshot serializer , swap ' and "(or just ' -> ") so we can make review easier

@thorn0
Copy link
Member

thorn0 commented Jan 29, 2020

The problem is it's easy to overlook something important in this batch of trivial changes.

@karlhorky
Copy link
Contributor Author

Apart from that, I would like to update this PR to avoid changing the CSS rules (CSS is more commonly written with double quotes).

I tried doing the following (in src/language-css/options.js), but it doesn't seem to make a change when I update the snapshots again with yarn jest -u --testPathPattern=css:

 "use strict";
 
-const commonOptions = require("../common/common-options");
+const CATEGORY_COMMON = "Common";
 
 // format based on https://github.com/prettier/prettier/blob/master/src/main/core-options.js
 module.exports = {
-  singleQuote: commonOptions.singleQuote
+  singleQuote: {
+    since: "0.0.0",
+    category: CATEGORY_COMMON,
+    type: "boolean",
+    default: false,
+    description: "Use single quotes instead of double quotes."
+  }
};

@kachkaev
Copy link
Member

Apart from that, I would like to update this PR to avoid changing the CSS rules (CSS is more commonly written with double quotes).

I believe that many teams using Prettier today with { "singleQuote": true } also like their CSS files using single quotes as well. If the option no longer applies to CSS, this will be a breaking change with no mitigation.

@nickmccurdy
Copy link
Member

nickmccurdy commented Jan 29, 2020

JSX

@nickmccurdy JSX is based on the visual appearance of HTML, which almost never has single quotes. So it's another language.

Still, Prettier should stay consistent with how it formats across file. Prettier uses double quotes in HTML and JSON files because they're required/recommended, so they should also be used in JS files. Prettier wouldn't use different indent levels in different languages when given the same config.

JSON itself, JSON.stringify

This is just one thing - JSON. Which, albeit related to JavaScript in many ways, is a different format (not even a language), meant to interop with many other languages. JavaScript is different enough to warrant a different decision.

The history of JSON used to be that it was very similar to JS, but now it's becoming a literal subset of modern ES specs. As a result, we can think of ECMAScript as being an extension of the JSON spec that adds scripting on the data.

not enough JS developers use them

Super interesting to hear that from someone - never heard this perspective before 1v1! Can you shed some light on where you get this from?

I have just finished a survey of many documentation pages of popular JavaScript libraries, maybe take a look. Single quotes almost everywhere: #4102 (comment)

Most of those packages are older packages for Node, which traditionally has more single quote usage relatively. On the other hand, most JavaScript usage is on the web with double quotes.

Also worth considering that Node uses single quotes in logs, while Chrome and Deno (which are arguably more modern at this point) use double quotes.

@karlhorky
Copy link
Contributor Author

karlhorky commented Jan 29, 2020

Still, Prettier should stay consistent with how it formats across file

This is exactly what I'm arguing against.

I believe a formatter for any language should do the most common, expected thing for the language. The language in question is JavaScript, no other language.

Most of those packages are older packages for Node

As I mentioned in my methodology over there in that comment, I included both older and newer popular libraries.

On the other hand, most JavaScript usage is on the web with double quotes.

@nickmccurdy Can you provide examples of these libraries with their public documentation with double quotes?

@@ -13,7 +13,7 @@ The first requirement of Prettier is to output valid code that has the exact sam

### Strings

Double or single quotes? Prettier chooses the one which results in the fewest number of escapes. `"It's gettin' better!"`, not `'It\'s gettin\' better!'`. In case of a tie, Prettier defaults to double quotes (but that can be changed via the [`--single-quote`](options.html#quotes) option).
Double or single quotes? Prettier formats to the one which results in a fewest number of escapes. `'We call it "Prettier".'`, not `"We call it \"Prettier\"."`. In case of a tie, Prettier defaults to single quotes (but that can be changed via the [`--single-quote`](options.html#quotes) option).
Copy link
Member

Choose a reason for hiding this comment

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

results in a fewest number sound wrong to me 🤔
IMO it should stay as results in the fewest number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed! This was an artifact from a previous wording that I changed back (but not completely!).

@pluma
Copy link

pluma commented Jan 30, 2020

Wait, how does this affect JSX? There was a huge kerfuffle when StandardJS started switching JSX literals to single quotes so I expect this PR doesn't do that, but will JS code in JSX files use single quotes too?

And what about Typescript? Most TS code I've seen uses double quotes. But having inconsistent formatting between TS and JS would be a little weird.

I also expect this PR won't affect CSS in template strings in JS/JSX/TS/TSX files because double quotes are the norm for CSS?

@karlhorky
Copy link
Contributor Author

karlhorky commented Jan 30, 2020

I expect this PR doesn't [switch to single for JSX]

@pluma Yes, I expect that it doesn't either. And it seems that it doesn't, in the playground

Typescript

TypeScript will also be formatted with singles. It's a superset of JS, so same rules as JS.

The Microsoft docs use double quotes - I've documented that in my ecosystem review. Is there anywhere else that uses doubles? I seem to recall seeing singles a lot with TypeScript code.

I also expect this PR won't affect CSS in template strings in JS/JSX/TS/TSX files because double quotes are the norm for CSS?

I would like to retain double quotes default for CSS, if possible: #7466 (comment) . Haven't figured out yet how.

@phpnode
Copy link
Contributor

phpnode commented Jan 30, 2020

imo this PR is not worth the disruption, people who like single quotes already have that option, why introduce this churn? I see the argument about the popularity of single quotes in JS, and maybe if prettier was brand new and didn't have the level of adoption it already has then this change would make a lot of sense, but this feels like "principle over pragmatism" to me. All this change will do is upset users.

@kachkaev
Copy link
Member

kachkaev commented Jan 30, 2020

At the moment, a need for the change is justified by:

To sum up, the only persuasion tactics currently used are ‘social proof’ and ‘authority’. Such methodology is quite shallow, which means that there is a chance of making a harmful decision in the end. I suggest doing a proper research before changing the default value.

@karlhorky
Copy link
Contributor Author

karlhorky commented Jan 30, 2020

@kachkaev you're missing out a lot of reasons. Really feels like you're operating in bad faith here, just based on your personal opinion:

@karlhorky
Copy link
Contributor Author

this PR is not worth the disruption
All this change will do is upset users

@phpnode This PR is meant for a major version bump, which in semver means breaking changes.

Luckily, for those who oppose the formatting changes to their codebase, there is still the option of writing a single line of configuration.

Not very much disruption at all, especially for a major version bump.

@phpnode
Copy link
Contributor

phpnode commented Jan 30, 2020

The point is that people who want to maintain the status quo (which is likely to be most people) will now need a configuration file when they didn't before. Most people are likely using prettier with default config, including in IDEs etc. It's making busy-work for thousands of devs for extremely questionable benefit.

It's not about whether single quotes are more prevalent in the eco-system, it's about whether churning something like this is worth it when the prettier user base is so large.

@karlhorky
Copy link
Contributor Author

karlhorky commented Jan 30, 2020

Most people are likely using prettier with default config

Is this just your opinion? Or do you have any data on that?

making busy-work
churning something like this is worth it

Like I mentioned above, the churn is negligible, especially for a major version bump. A 30 second commit after an upgrade for a team is acceptable.

extremely questionable benefit

Maybe in your opinion. There are a long list of reasons and people speaking for this change.

@phpnode
Copy link
Contributor

phpnode commented Jan 30, 2020

Of course I don't have any data on that, and I don't see an acceptable way for anyone to collect that data, but it makes sense, no? It's an opinionated code formatter which works out of the box, I think it's very very likely that most people won't have bothered to configure it. This was the original selling point of prettier!

@pluma
Copy link

pluma commented Jan 30, 2020

I for one welcome our new single quote overlords even if single quotes are normally annoying to type on a QWERTZ keyboard layout (both require chords but single quotes are on the right hand side).

Let's not kid ourselves about the public vote here though. Twitter likes are irreverent and ~200 GitHub reactions aren't representative for a project with this many installs.

The real reason isn't the votes but that this has been a long-standing wish within the prettier dev team based on the widespread preference within the JS open source ecosystem. That is absolutely legit but I'd prefer we stick to that rationale rather than democracy-wash what is clearly not intended (nor required) to be an opinion poll or popularity contest.

EDIT: feel free to mark my earlier comment as resolved, my concerns have been addressed.

@lydell
Copy link
Member

lydell commented Jan 30, 2020

I for one welcome our new single quote overlords even if single quotes are normally annoying to type on a QWERTZ keyboard layout (both require chords but single quotes are on the right hand side).

That’s irrelevant when using Prettier. You can type whichever quote is easier and Prettier will fix them for you.

@peterjuras
Copy link

How will this behave in IDEs like VSCode? Right now I can copy and paste a JavaScript object /array from a JS file into a JSON file and the prettier extension will format it to make it JSON compatible.

Will this still work 100% of the times? Or will it change everything to single quotes when the file format is not 100% clear, thus breaking JSON?

I can understand breaking changes that reformat long lines at different breakpoints for higher (supposed) readability. But this change doesn't make sense to me since it's just an opinionated change which could be reverted in a few years because double quotes might get cool again.

I don't want to use or introduce a configuration file with prettier. Keeping it in sync across multiple repositories in a team is a pain. Therefore the argument of "you can easily configure this" is not working for me, it would rather completely invalidate this PR because if this would be the case then you would simply apply a single quite configuration to your repos and would have no need to change the default style.

I'm indifferent about whether this makes into the core of prettier our not, but in case that I do feel any downgrade in editor experience when dealing with JSON, it would leave a really bad impression about prettier for me.

@pgsandstrom
Copy link

@peterjuras Prettier can format a json-file using single quote to using double quote. So there should be no problem copy-pasting javascript-objects to json-files.

@glen-84
Copy link

glen-84 commented Jan 31, 2020

Please consider @Shinigami92's suggestion before making such a drastic change.

I won't argue about single quotes being more common in JavaScript, but Prettier is used with many other languages as well. The default should not be based on what is most common in JavaScript, but by what is most common across all languages.

A probably-common case of JavaScript (single), HTML (double), and CSS (double), would now require an override with 2 file extensions instead of 1. I can't imagine someone using single quotes for HTML, so even if you only needed that one override, you'd just be swapping *.js for *.html – what does that achieve?

@alexander-akait
Copy link
Member

alexander-akait commented Jan 31, 2020

/cc @prettier/core It is very hot in here, I am afraid that using single quotes will bring more pain than improvements, my suggestion is to postpone that to version 3.0 and investigate this issue in detail.

Perhaps we hastened with some decisions about this issue.

@alexander-akait
Copy link
Member

/cc @thorn0 @lipis @j-f1

@thorn0
Copy link
Member

thorn0 commented Mar 22, 2020

I unintentionally closed this and several other PRs because I deleted the next branch, but let's keep this one closed for now.

@karlhorky
Copy link
Contributor Author

karlhorky commented Mar 23, 2020

@thorn0 @lipis @j-f1 @lydell @evilebottnawi @Shinigami92 Should this be rebased on the 2.0 branch now and a new PR opened?

I think it probably shouldn't just stay closed just because it was accidentally closed...

@wouterds
Copy link

wouterds commented Mar 23, 2020

I think it probably shouldn't just stay closed just because it was accidentally closed...

I agree.

Edit: I've been using prettier for years and the only customization I always have in my .prettierrc is

{
  "singleQuote": true,
  "trailingComma": "all"
}

@thorn0
Copy link
Member

thorn0 commented Mar 23, 2020

Should this be rebased on the 2.0 branch now and a new PR opened?

Only if there is a final decision about changing this default, I guess. Otherwise, what for?

@karlhorky
Copy link
Contributor Author

karlhorky commented Mar 23, 2020

Only if there is a final decision about changing this default, I guess. Otherwise, what for?

To have a PR that can be easily merged if and when the decision is made to merge (for example, for 3.0).

@karlhorky
Copy link
Contributor Author

Even though more than a few people and their friends / colleagues supporting double quotes suddenly appeared in this issue to make this into a heated discussion, it seems like given the numbers that they are the vocal minority - which seems to me should not be the reason that Prettier decides to do or not do something.

I still feel that given the research that has been already done, this change to a default single quote would better fit JavaScript and benefit the majority of developers who write it.

Every time a poll has been done, the majority supports this choice.

If the research is required for 3.0 that a larger, more wide reaching and impartial poll or survey of existing code be done (which I believe is overblown for a default on a configurable option), then I'm sure that the numbers would also tell the same story as they have up until now.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet