-
-
Notifications
You must be signed in to change notification settings - Fork 405
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
quality: add flake8-quotes #2322
Conversation
I re-read the diff, just to be extra-sure, and I didn't see any mistake. I followed the preferences of @dgw written on #1765 about this flake8 plugin (which is: to use the default behavior). I might want to ask for a fast track merge on this one, or this will increase the number of conflicts and new flake8 errors later on, since it touches almost everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll say again that I disagree with the premise, I much prefer double quotes over typing a string then noticing it has an apostrophe and needing to replace the singles with doubles. Apostrophes are a markedly more common case than nested quoting.
sopel#flake8-quotes$ git grep '".*'"'"'.*"' | wc -l # How many "...'..."
213
sopel#flake8-quotes$ git grep "'"'.*".*'"'" | wc -l # How many '..."...'
126
Not that the world should cater to C, but for anyone who's used a language like it, anything but double quotes on a string is something to trip over constantly.
That said, besides the actionable comments, this does what it says on the tin.
"Warning: %s is an uncommon operating system platform. " | ||
'Warning: %s is an uncommon operating system platform. ' | ||
"Sopel should still work, but please contact Sopel's developers " | ||
"if you experience issues." | ||
'if you experience issues.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixing in one ('...' "..." '...') is gross
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spy zheller/flake8-quotes#82 from *checks notes* three years ago, about this. I think we're stuck with it. :/
^(\d+(?:\.\d+)?) # Decimal number | ||
\s*([a-zA-Z]{3}) # 3-letter currency code | ||
\s+(?:in|as|of|to)\s+ # preposition | ||
(([a-zA-Z]{3}$)|([a-zA-Z]{3})\s)+$ # one or more 3-letter currency code | ||
''', re.VERBOSE) | ||
""", re.VERBOSE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why it prefers doubles in multiline strings where it matters the least
'thousands of Reddit communities.') | ||
elif match_lower == 'popular': | ||
public_description = ('The top trending content from some of ' | ||
'Reddit\'s most popular communities') | ||
"Reddit\'s most popular communities") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Reddit\'s most popular communities") | |
"Reddit's most popular communities") |
Does this flake plugin not catch these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah weird, it didn't catch this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's designed to catch this. zheller/flake8-quotes#98
Just another reason I'm disappointed in how useful this plugin turned out to be, and wish I'd experimented with it before adding to the wishlist so Exirel didn't waste time implementing. 😦
'Error parsing JSON response from translate API (%s to %s: "%s")', | ||
"Error parsing JSON response from translate API (%s to %s: '%s')", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that might have been either an honest mistake (I did some find & replace regex), or there is something else. I think it's probably a mistake.
"domain_blocklist_url", | ||
"Optionally, provide the URL for a hosts-file formatted domain " | ||
'domain_blocklist_url', | ||
'Optionally, provide the URL for a hosts-file formatted domain ' | ||
"blocklist to use instead of StevenBlack's.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same gross
What gets me about your figures is actually how common In reality, I don't mind double quotes in most cases. If I really didn't like them, there are several @half-duplex PRs I would have blocked. 😝 (I mostly remember calling h-d out on changing quotes in lines that didn't otherwise need to be touched.) Looking back at the text @Exirel cited from me, I think that was an incomplete expression of preference—for which I owe everyone who's looked at the list an apology. Where double quotes bug me is in subscripts ( This PR may have nudged me away from the idea of using this plugin after all, since it doesn't appear possible to configure it such that my exact preferences would work. The only distinction it makes is between Unless I'm missing something, we can't have this plugin enforce all of these with one configuration (examples could be more comprehensive, but I ran out of ideas): # dict keys and similar with singles
some_dict['keyname']
# output string with doubles, but a short parameter with singles
bot.say("Some {} message.".format('useless'))
# short parameter with singles
helper_func(varname, 'truth_check', 30)
# continuation with doubles (actually the plugin probably *could* do this if `double` is the default)
long_helper(
"Something wicked this way comes. "
"(It's a witch.)"
) I just wish this realization had come from experimenting with the cleanup myself and trying to make this plugin do what I want it to do, rather than taking @Exirel's time for that. 😢 |
Yeah that makes sense. I think we should close this for now, we might want to give it another try later in the future, when we don't have 30 other issues to fix before releasing Sopel 8 then. I think it was worth the try to show what it means, in term of diff and efforts. |
As per IRC discussion: we close this one for now! |
Description
Tin. This was a nightmare. Please never again. Good luck reviewers.
Checklist
make qa
(runsmake quality
andmake test
)