quality: add flake8-quotes #2322

Exirel · 2022-07-14T15:18:58Z

Description

Tin. This was a nightmare. Please never again. Good luck reviewers.

Checklist

I have read CONTRIBUTING.md
I can and do license this contribution under the EFLv2
No issues are reported by make qa (runs make quality and make test)
I have tested the functionality of the things this change touches

Exirel · 2022-07-14T15:33:47Z

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.

half-duplex

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.

half-duplex · 2022-07-14T17:29:57Z

sopel/cli/run.py

-            "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.'


Mixing in one ('...' "..." '...') is gross

I spy zheller/flake8-quotes#82 from *checks notes* three years ago, about this. I think we're stuck with it. :/

half-duplex · 2022-07-14T17:51:47Z

sopel/modules/currency.py

    ^(\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)


I wonder why it prefers doubles in multiline strings where it matters the least

half-duplex · 2022-07-14T18:01:15Z

sopel/modules/reddit.py

                                  '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")


Suggested change

"Reddit\'s most popular communities")

"Reddit's most popular communities")

Does this flake plugin not catch these?

Yeah weird, it didn't catch this one.

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. 😦

half-duplex · 2022-07-14T18:05:28Z

sopel/modules/translate.py

-            'Error parsing JSON response from translate API (%s to %s: "%s")',
+            "Error parsing JSON response from translate API (%s to %s: '%s')",


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.

half-duplex · 2022-07-14T18:17:02Z

sopel/modules/safety.py

-        "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.",


dgw · 2022-07-15T01:59:25Z

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.

What gets me about your figures is actually how common '..."...' is. I expected it to be rarer. I guess we use a lot of that pattern for logging and/or command-usage messages.

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 (some_array['key']), some function args (one-word strings), and other code-y places. For logs, bot output, etc., double quotes are great.

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 inline and multiline quotes, and that just doesn't match my (weird) desire for (questionable) consistency.

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. 😢

Exirel · 2022-07-17T11:04:50Z

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.

Exirel · 2022-07-17T11:33:36Z

As per IRC discussion: we close this one for now!

Exirel added 4 commits July 14, 2022 17:15

sopel: prepare for flake8-quotes (except sopel.modules)

4bb0b55

sopel.modules: prepare for flake8-quotes

1f38ccb

test: prepare for flake8-quotes

c909343

dev: add flake8-quotes

a781c61

Exirel added the Housekeeping Code cleanup, removal of deprecated stuff, etc. label Jul 14, 2022

Exirel added this to the 8.0.0 milestone Jul 14, 2022

Exirel mentioned this pull request Jul 14, 2022

Additional flake8 plugins we'd like to use #1765

Open

9 tasks

half-duplex requested changes Jul 14, 2022

View reviewed changes

Exirel closed this Jul 17, 2022

dgw removed this from the 8.0.0 milestone Jul 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quality: add flake8-quotes #2322

quality: add flake8-quotes #2322

Exirel commented Jul 14, 2022

Exirel commented Jul 14, 2022

half-duplex left a comment

half-duplex Jul 14, 2022 •

edited

dgw Jul 15, 2022

half-duplex Jul 14, 2022

half-duplex Jul 14, 2022

Exirel Jul 14, 2022

dgw Jul 15, 2022 •

edited

half-duplex Jul 14, 2022

Exirel Jul 14, 2022

half-duplex Jul 14, 2022

dgw commented Jul 15, 2022

Exirel commented Jul 17, 2022

Exirel commented Jul 17, 2022

	"Reddit\'s most popular communities")
	"Reddit's most popular communities")

		'Error parsing JSON response from translate API (%s to %s: "%s")',
		"Error parsing JSON response from translate API (%s to %s: '%s')",

quality: add flake8-quotes #2322

quality: add flake8-quotes #2322

Conversation

Exirel commented Jul 14, 2022

Description

Checklist

Exirel commented Jul 14, 2022

half-duplex left a comment

Choose a reason for hiding this comment

half-duplex Jul 14, 2022 • edited

Choose a reason for hiding this comment

dgw Jul 15, 2022

Choose a reason for hiding this comment

half-duplex Jul 14, 2022

Choose a reason for hiding this comment

half-duplex Jul 14, 2022

Choose a reason for hiding this comment

Exirel Jul 14, 2022

Choose a reason for hiding this comment

dgw Jul 15, 2022 • edited

Choose a reason for hiding this comment

half-duplex Jul 14, 2022

Choose a reason for hiding this comment

Exirel Jul 14, 2022

Choose a reason for hiding this comment

half-duplex Jul 14, 2022

Choose a reason for hiding this comment

dgw commented Jul 15, 2022

Exirel commented Jul 17, 2022

Exirel commented Jul 17, 2022

half-duplex Jul 14, 2022 •

edited

dgw Jul 15, 2022 •

edited