Add flake8-quotes to linting workflow

[Numerlor]

Double quotes are currently preferred by the style guide, but aren't enforced in any way in the project. This leaves the project in a bit inconsistent state ands adds more work for reviewers and contributors during the PR process.

The flake8-quotes plugin can be used to raise linting errors if the preferred quote type is not used. From testing, everything seems to work fine except for multi line strings with implicit concat zheller/flake8-quotes#82, but this is easy to ignore and I haven't found any uses in a quick look through this project.

The main deterrent to this change would be the change to the existing codebase, at the time of writing the main branch raised
1044 errors, on 773 lines in 71 unique files. 14 of these files and 511 errors are in files under the tests folder.
While it would be a big change, it can be done as a mechanical process and merged quickly after the change if everything is discussed, so the long term disruption should be minimal.

[Akarys42]

I'm not sure if we really want to change them all at once, mainly for all the conflicts it will arise. We can just change them as we go on, and perhaps look at this once we only have a few double quotes remaining?

I am personally not a fan of the rule that enforces double quotes, I use each type of quote depending on the context. Also, this does not automatically change the quotes, which is pretty silly if you ask me, I'd rather have it as a precommit hook.

Poke @Xithrius, @jb3, @HassanAbouelela

[Numerlor]

I'm not sure if we really want to change them all at once, mainly for all the conflicts it will arise.

From a quick replace I found conflicts when merging #1602, #1568, #1539, #1487 and #836; it affects less PRs than I thought it would, but still a considerable number. Although conflicts coming from here should be mostly trivial to resolve.

We can just change them as we go on, and perhaps look at this once we only have a few double quotes remaining?

I really doubt this will ever happen, the existing code doesn't get change as much and quotes are easy to miss during reviews.

I am personally not a fan of the rule that enforces double quotes, I use each type of quote depending on the context.

The plugin will properly choose quotes to avoid escaping etc. in other cases the quotes would be against the style guide so I don't think those should be accommodated.

Also, this does not automatically change the quotes, [...], I'd rather have it as a precommit hook.

I didn't consider that, but it sounds better as quotes can differ between projects so having it taken care of automatically would be nice.
There is the double-quote-string-fixer hook but it works the other way around so we'd need to change it and either have it locally or have a repo for it (I haven't found anything that already does this). After the quote change it seems to work in the same way as the flake8 plugin.

I've also noticed that neither the plugion or the precommit hook will enforce the quotes within multi line f string literals where

f"""{'a'}"""

doesn't get flagged or changed

[HassanAbouelela]

I agree with Numerlor about doing this all at once.

Regarding hook vs flake plugin, I prefer the plugin because I don't want us to tie ourselves up in maintaining and modifying the hook. A single person's changes will probably be minimal, and hopefully, they'll drop to 0 as they become more familiar with our code style. Manual compliance with the plugin is not all that difficult.

To get over the initial hurdle of converting everything, I made a short script to go over the output of the flake plugin, and clean everything up. I tested AST equivalence, ran the test suite, and started the bot, and saw no problems.

The script is here, and my changes have been pushed to this branch. (Pffft, who needs git history anyways)

[Numerlor]

Regarding hook vs flake plugin, I prefer the plugin because I don't want us to tie ourselves up in maintaining and modifying the hook. A single person's changes will probably be minimal, and hopefully, they'll drop to 0 as they become more familiar with our code style. Manual compliance with the plugin is not all that difficult.

While I agree that it is a non issue after the first run, using the hook could make it easier for contribs, where they'd use their personal style that'd then be automatically changed at the end.
Apart from having to have the code somewhere I don't think maintaining it should be an issue as the file only got 10 commits in its entire 6 year history and I don't anticipate the grammar around strings changing anytime soon.

If changing out the few quote literals in the file gets the equivalent changes as the flake8 plugin I think it could be worth it, but both of the options are better to me than continuing to prolong the mixed style in the repo

[jchristgit]

We have talked about this internally and we decided on not adding a new linting dependency here. While there's some value in having linters for consistent code like this, there is a line to draw - it's not a great experience for contributors to have a contributor go back and modify their code just for changing the quotation style, especially since some developers tend to have fixed habits on their usage of quotes in Python.

[Numerlor]

We have talked about this internally and we decided on not adding a new linting dependency here. While there's some value in having linters for consistent code like this, there is a line to draw - it's not a great experience for contributors to have a contributor go back and modify their code just for changing the quotation style, especially since some developers tend to have fixed habits on their usage of quotes in Python.

What about the local pre-commit which would do it automatically along with the other checks like trailing spaces?

More so on the developers having habits on their own quoting style, how should this be handled in reviews?
The current Python Discord style is to use ", should this be pointed out in reviews (i.e. doing the same thing as the linting check but in a different place and by a human) or not?

[jchristgit]

What about the local pre-commit which would do it automatically along with the other checks like trailing spaces?

I am indifferent about this, personally the pre-commit setup has grown to a size where I have completely removed it from my workflow due to the time it needs to run through.

More so on the developers having habits on their own quoting style, how should this be handled in reviews?
The current Python Discord style is to use ", should this be pointed out in reviews (i.e. doing the same thing as the linting check but in a different place and by a human) or not?

In my opinion these comments are of little value for the code review, because they often state personal preference rather than pointing out actual topics regarding the code submitted.

[Numerlor]

Is it personal preference if it's the project's style set by the style guide?
To be clear, I agree that pointing these out in code reviews is nitpick-y, but the mixed style in the codebase is not great which is why I suggested incorporating this into the linting (or fixing it through pre-commit)