Unignore flake8 E123, E126, E305, E741, W504

[arogl]

Un-ignore flake8 errors that require more than zero or minimal change to the codebase.

See the the discourse discussion

Uploading changes based on PR #3666 and #3669 for discussion

To Do

• Changelog. (Add an entry to docs/changelog.rst near the top of the document.)

[arogl]

Sorry for the spamming

[sampsyo]

Looks wonderful; thank you for getting this heroic effort going! The whole "binary operators go on the second line, not on the first line" thing is going to take some getting used to, but I think I can adapt. 😅

I did a complete review of all the changes. A couple of things seem like they're worth addressing before we merge:

• Some of the automatically "fixed" variable names could use an audit.
• Some new indentation choices are somewhat strange (presumably these were also automated?).

[sampsyo]

Weird—I think this conformant indentation is now less clear, somehow? Maybe it would look less strange if we just put the closing parentheses right after the '1' instead of on their own lines.

[sampsyo]

Another instance where I think the new conformant indentation is unambiguously worse… maybe the checker would be happy if we just un-indented the last line to match the first line?

[sampsyo]

Maybe log would be a clearer name for this variable?

[sampsyo]

As above.

[sampsyo]

I'm not really sure fixing this single-letter variable name (in this test file) is really worth it. But if we do, let's pick something else simple instead of new names based on the surrounding context.

[sampsyo]

Perhaps a better name for this variable would be out or something to indicate that it's the output log from the command.

[sampsyo]

Perhaps a better name for this argument would be lst (the l is short for "list").

[jtpavlock]

Regarding the indentation changes, I tend to agree that they seem a little strange. For comparison, I ran the relevant code blocks through the black python code formatter, and it chose some different indentations.

             """.format(
-                model_cls._flex_table,
-                model_cls._table,
-                where or '1',
-            )
+            model_cls._flex_table, model_cls._table, where or "1",
         )

and

-            num = int(command_output([
-                '/usr/sbin/sysctl',
-                '-n',
-                'hw.ncpu',
-                ]).stdout)
+            num = int(command_output(["/usr/sbin/sysctl", "-n", "hw.ncpu",]).stdout)

Perhaps, it could be useful to switch to using black as a code formatter entirely? Then we wouldn't have to worry about these linting errors or correcting user's, as all the code would be unified.

Edit: I just realized this was also with black's default max line-length of 88

You probably noticed the peculiar default line length. Black defaults to 88 characters per line, which happens to be 10% over 80. This number was found to produce significantly shorter files than sticking with 80 (the most popular), or even 79 (used by the standard library). In general, 90-ish seems like the wise choice.

Using it with max line-length of 80 results in a different diff for the second one:

-            num = int(command_output([
-                '/usr/sbin/sysctl',
-                '-n',
-                'hw.ncpu',
-                ]).stdout)
+            num = int(
+                command_output(["/usr/sbin/sysctl", "-n", "hw.ncpu",]).stdout
+            )

[arogl]

Have some time to come back to this.
I reformatted the entire db.py file using black for comparison with these options:
--line-length 79
--skip-string-normalization

I can run the entire code base through black if we wish to compare and have a conversation about a formatter and the default settings.
If we are going to python3 only support we could reformat the entire code base in another branch.

I'll tackle the variable names in the tests later today. I was reluctant to change those as they were tests. This depends how conformant we wish to be.

I've run the attached diff across beets/master using

black --diff --skip-string-normalization --line-length=79 . > black.diff

black.diff.txt

[jtpavlock]

Thanks for keeping up with this. I think whether or not we decide to switch to using a universal code formatter like black probably deserves it's own discourse discussion. If we do adopt something like that, I imagine these flake8 errors would disappear as a side effect.

[arogl]

I was just using black as a comparison, not saying we should apply it completely or anything. Whether or not to use black probably deserves it's own discourse discussion. I imagine if we do switch though, these flake8 errors would go away as a side effect.

I had done all of the indenting manually.

The black format would fix it once if we accept the changes.

I still have to redo the variables

Now I need to work out how to fix the extra commit I did with the fetch of master 😰

[jtpavlock]

Yeah the variables are formatting independent, so that's good. Perhaps that could go in its own pull request while we decide what to do about the more sweeping formatting changes?

[jtpavlock]

Also, here's a great article on adjusting some commits. I can't believe I used git for years before I found out about git rebase -i

[jtpavlock]

Sorry for spamming this PR, but I started a discussion on discourse about black.

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.