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

Consider being even more opinionated about quotes #51

Closed
dusty-phillips opened this issue Mar 21, 2018 · 21 comments
Closed

Consider being even more opinionated about quotes #51

dusty-phillips opened this issue Mar 21, 2018 · 21 comments
Labels
help wanted Extra attention is needed T: enhancement New feature or request

Comments

@dusty-phillips
Copy link

dusty-phillips commented Mar 21, 2018

One thing I've come to appreciate from JS land is that even though ' and " are interchangeable, you can set the linters to require one or the other. My Python code is littered with alternating ' and " strings, sometimes even in the same dictionary, like so:

{
    'My key': "has a value",
    "my quote": 'is doubled',
}

I don't have an opinion on which is best (and I don't want to), but I wish to hell my code was consistent. There are good reasons to use one or the other (if the string contains other quotes, for example), but in general, I wish they were all the same. Maybe it could adjust any string that is enclosed in ' characters and does not contain a " character to be wrapped with " characters (or vice versa):

e.g
'hello' becomes "hello"
'She said "yes"' doesn't change
"Don't do that" never changes.

Should also be able to change all triple quotes to the same character as well unless the string contains the inverse quote.

@ambv ambv added T: enhancement New feature or request help wanted Extra attention is needed labels Mar 21, 2018
@ambv
Copy link
Collaborator

ambv commented Mar 21, 2018

I like what you're suggesting.

@sethmlarson
Copy link
Member

I would love it if this was configurable somehow because I've seen many projects which use primarily use single quotes instead of double quotes.

@ambv
Copy link
Collaborator

ambv commented Mar 23, 2018

I would just default to single quotes everywhere unless otherwise required. Rationale: single quotes are easier to type.

But I'd leave """ for multiline strings since it's more idiomatic than ''' which just looks foreign.

Note: normalizing this can be a start of #26.

@sethmlarson
Copy link
Member

sethmlarson commented Mar 23, 2018

@ambv Completely agree with your rationale. Maybe there can be a detector based on frequency and then forces it to one or the other? 😛 Otherwise just defaulting to single-quotes is fine by me.

@zsol
Copy link
Collaborator

zsol commented Mar 24, 2018

This is going to get complicated for mixed quotes, and f-strings are also an interesting use case. Here's my understanding what we want:

input formatted
"Hello" 'Hello'
"Don't do that" unchanged
'Here is a "' unchanged
'What\'s the deal here?' "What's the deal here?"
"What's the deal \"here\"?" 'What\'s the deal "here"?
"And \"here\"?" 'And "here"?'
"""Strings with "" in them""" unchanged
'''Here's a ""''' """Here's a """""
f'MOAR {" ".join([])}' unchanged
f"MOAR {' '.join([])}" unchanged

So to summarize:

  • prefer ' over "
  • prefer """ over '''
  • never introduce more escaping
  • just leave f-strings alone.

@ambv
Copy link
Collaborator

ambv commented Mar 24, 2018

Thanks for your interest in solving this!

If you want a 100% solution, you're right that this has ugly edge cases. How about a first iteration (we're still alpha, right?) that would only do the following:

  • change " to ' and ''' to """ (including in f-strings)
  • if and only if there are no escapes and no quotes in the body of the string.

That will already fix a lot of inconsistencies in user files and is a great start for further tweaks later.

@zsol
Copy link
Collaborator

zsol commented Mar 24, 2018

Sounds good, I can take a crack at this first step.

We'll still need to figure out the edge cases in the future ;)

@ambv ambv added this to In Progress in Getting to beta Mar 24, 2018
@sersorrel
Copy link
Contributor

As someone who primarily uses double quotes for strings, I'd be a bit sad if Black started enforcing single quotes everywhere. My own reasons for using double quotes are, as far as I can tell (in no particular order):

  • Docstrings (and triple-quoted strings in general) almost exclusively use double quotes, per PEP 8, and using different quotes for docstrings and normal strings doesn't make much sense to me.
  • Double quotes are more visible than single quotes (it's easier for me to spot a stray " than a ').
  • Similarly to the above, it's possible to confuse " for '', especially in variable-width fonts: if you write the empty string as '', it's possible to confuse it with " at a glance, but nobody will confuse "" with anything.
  • C uses double quotes for strings.
  • Within strings, I use single quotes (for contractions like "don't") much more than I use double quotes, and no matter what PEP 8 says, switching between ' and " based on the contents of the string always feels a bit awkward.
  • I think, although it's hard to tell, that it's actually easier for me to type double quotes than single quotes; my right pinky is normally somewhere near backspace, whereas my left middle finger is usually right next to " (it's probably worth noting that I'm using a UK keyboard!).

It would be nice if there was a config option for this, if only to save some UK programmers' right pinky fingers some work :)

@ambv
Copy link
Collaborator

ambv commented Mar 25, 2018

Black's mission is to make all blackened code look the same. So if we decide to standardize quotes used, we won't be providing an option to do the opposite thing.

As far as your reasoning goes, you list a few things which I strongly disagree with. If you are ever confusing a double quote for an empty single-quoted string, or if you ever have a problem spotting a stray apostrophe, I would suggest abandoning Microsoft Word and adopting a code editor that highlights your syntax and uses a fixed-width font 😏

Smirk aside, I can't disagree with the docstring argument, hence I think triple quotes should be using """. I also agree that starting a string with " makes it apostrophe-proof 😄 I also understand the UK keyboard argument, I think this also applies to the German layout. We could play the numbers game about which layout is more often used and I'd be surprised if the US (aka international) layout wouldn't win by a large margin. But I think in this case we can eat the cake and have it, too.

Do you think it would make much difference to you if you kept typing your double quotes and Black just silently converted them to single quotes with no extra labor on your side?

@sersorrel
Copy link
Contributor

Do you think it would make much difference to you if you kept typing your double quotes and Black just silently converted them to single quotes with no extra labor on your side?

That was something I considered, I think it would probably work ok. Or I could just start using single quotes, since I'm already typing them often anyway :P

I'm not totally convinced, but if that's what Black does, then that's what Black does.

zsol added a commit to zsol/black that referenced this issue Mar 25, 2018
Convert simple double-quoted strings to single-quoted. Convert triple (single) quoted strings to triple (double) quoted. Do not touch any strings that have backslashes or quotes inside the string.

Fixes psf#51.
@carljm
Copy link
Collaborator

carljm commented Mar 25, 2018

I think we should be very hesitant to add scope to Black, and only do it if we are quite sure the result is actually almost always improved consistency. We have plenty of work to do to fix bugs within the existing scope, there’s no need to be eager to expand the scope, it mostly just expands the list of scope for bugs and the list of reasons for someone to dislike Black. The cost of saying “Black doesn’t mess with your quotes, quote as you like” is very low. The cost of bad/buggy/inconsistent rewriting of quotes is much, much higher.

@carljm
Copy link
Collaborator

carljm commented Mar 25, 2018

IMO the only quote transformation that is a clear win is transforming all triple single quotes to triple double quotes.

@sethmlarson
Copy link
Member

I thought the scope of black is to make all code look homogenous. I'd say quotes fall under that?

zsol added a commit to zsol/black that referenced this issue Mar 25, 2018
Convert simple double-quoted strings to single-quoted. Convert triple (single) quoted strings to triple (double) quoted. Do not touch any strings that have backslashes or quotes inside the string.

Fixes psf#51.
@ambv
Copy link
Collaborator

ambv commented Mar 26, 2018

@carljm, thanks for your insightful comments here and on the pull request at #75. I'll share some more of my long-term plan and you can all tell me what you think.

We're going to mess with strings in Black. We're going to join implicitly concatenated string literals (see #26). To be able to do this, we will have to normalize quotes (and prefixes if it doesn't matter). If we're already doing that, let's do that everywhere. In fact, we're going to fight implicit string concatenation because it causes site outages.

More generally, Black is different from existing propositions for Python because it's not afraid to modify the AST beyond whitespace. We're going to be adding and removing parentheses when it makes sense, which will help with wrapping lines in some cases. We are already making trailing commas more consistent. String literals are just another category in the same vein.

And a total bird's eye view is this. Black is not just a machine code formatter. It is a code style. When a project sports the Code style: black badge, it doesn't just mean "I use this tool". It tells people that this project uses the same style as all blackened code. That's why the badge says "code style", not "formatter".

If this code style consistently makes teams develop 1% faster, debug 1% faster, and the code 1% less buggy, discussions about whether a single quote or a double quote looks nicer become laughable. And I think 1% is conservative.

@gaggle
Copy link

gaggle commented Mar 26, 2018

Just to add €0.02, I use double-quotes for similar reasons that @anowlcalledjosh mentions:

  • I tire of adding a concatenated word to a string only to then also have to change the quotes. It makes the diffs larger than they need to be. I've internalised using double-quotes from the beginning.
  • Matches established docstrings pattern.
  • IMO more visibly obvious, but eh that's getting rather subjective.

OTOH black's cousin StandardJS enforces singe-quotes. And it's fine. My double-quote habit in JS broke after not even an hour.

I'll happily accept either way you all decide to go for it, ultimately a final decision here saves us all from bikeshedding this tiny decision forever. No pressure :)

@carljm
Copy link
Collaborator

carljm commented Mar 26, 2018

Commented at #75 (comment)

tl/dr inconsistent string quoting is a maintenance problem, not an aesthetic problem, and the combination of preferring single quotes and avoiding escapes at all costs results in Black creating lots of inconsistent string quoting, rather than eliminating it.

@ambv
Copy link
Collaborator

ambv commented Mar 26, 2018

I agree with Carl's comment on the pull request and thus reversed my opinion: we will default to double quotes. Especially given my own argument that if they are harder to type, just keep typing single quotes and Black will fix it for you.

zsol added a commit to zsol/black that referenced this issue Mar 31, 2018
Convert simple double-quoted strings to single-quoted. Convert triple (single) quoted strings to triple (double) quoted. Do not touch any strings that have backslashes or quotes inside the string.

Fixes psf#51.
@ambv ambv closed this as completed in #75 Mar 31, 2018
Getting to beta automation moved this from In Progress to Done Mar 31, 2018
ambv pushed a commit that referenced this issue Mar 31, 2018
* Normalize string quotes

Convert single-quoted strings to double-quoted. Convert triple single-quoted strings to triple double-quoted. Do not touch any strings where conversion would increase the number of backslashes.

Fixes #51.

* reformat Black itself
@joslarson
Copy link
Contributor

@ambv I know this is closed, and I understand the arguments above, but I wonder if we should consider reopening this discussion to consider the following two points:

  1. Python's global repr function defaults to rendering strings with single quotes.
  2. I'd have to verify this, but from my experience, most popular open source Python projects I've run into default to single quotes as well.

A quick spot check shows these libraries as defaulting to single quotes: Flask, Django, Requests, PEP8 docs. I'm sure there are plenty that don't, but It might be worth investigating to get some hard data.

@ambv
Copy link
Collaborator

ambv commented May 19, 2018

  1. How is it relevant what repr() defaults to?
  2. They not so much "default to" a single quote style as rather use them inconsistently. Some projects might claim a particular style but I haven't seen it enforced consistently anywhere. Oh, and by the way, Requests 3.0 isn't released yet but it's formatted with Black.

@joslarson
Copy link
Contributor

@ambv I considered both of the things I mentioned relevant in that I didn't see anything in the discussion above considering what is the most commonly seen style is across the Python community. I think there's something to be said for precedent when it comes to creating a unified code style.

In my corner of the Python world it seems like I look at a lot more Python source code defaulting to single quotes for everything but doc strings, so I thought it would be worth discussing.

@alanhamlett
Copy link

alanhamlett commented May 26, 2018

Requests 3.0 isn't released yet but it's formatted with Black.

That's a circular definition.

Forcing double quotes without an option for single quotes means blocking teams from using Black. How about changing Black's mission statement to:

Make all blackened code look the same, with some ambiguous styles not defined in PEP8 up to the individual project.

@psf psf locked and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Extra attention is needed T: enhancement New feature or request
Projects
No open projects
Development

No branches or pull requests

9 participants