Huge +1 for covering Python with AN automated tool for formatting and linter. Although, I am not proficient in those tools, hence replacing myself with Sybren for the final review.

One thing I'll mention is that some of the errors which are listed in the ignore is something we should NOT ignore in the long term. It is fine, however, to ignore those for an initial tool integration (similar to Clang-Tidy), but be clear of what we "permanently" ignore and what we ignore during "transition" time.

Also question: is it something we impose on the addons? addons-contrib? Shall the config be in the submodule itself (maybe, as well)?

Like Sergey, huge +1 for auto-formatting & auto-fixing of Python code.

we could also add this in the root directory, I don't have a strong opinion about this

I do have a strong opinion about this, and that is that we totally should add this file to the root directory. The files in tests/python, tests/performance,release/datafiles, and others should be covered by the same configuration.

This project is also intended to be used as a kind of project-meta-data file for Python projects which is not our intended use case, so I prefer not to make it so prominent.

I disagree here as well. Tools like mypy will also look here for their configuration, and the use of such a tool can help to avoid lots of bugs that are otherwise only discoverable at run-time. Having this file at the project root opens the doors for more interesting tooling that we can use to increase the quality of our Python code.

In D14686#394726, @Sergey Sharybin (sergey) wrote:

One thing I'll mention is that some of the errors which are listed in the ignore is something we should NOT ignore in the long term. It is fine, however, to ignore those for an initial tool integration (similar to Clang-Tidy), but be clear of what we "permanently" ignore and what we ignore during "transition" time.

Another +1 here.

Per ignore rule, this is my personal opinion on things. Again, I'm fine if we have this list initially, but I think that all ignores can be removed eventually.

E721: The number of cases where direct type comparison is correct, compared to the number of cases where people don't know isinstance() is the right thing to use, is heavily leaning towards the latter. Not all Blender developers are equally skilled in Python (which in itself is absolutely fine of course). This rule would make doing the right thing easier, and thus IMO should not be ignored.

E722: Bare except clauses must be avoided. Only when they are absolutely necessary should they be used, and in those very rare cases we can locally ignore the error and document the reasons why to make it abundantly clear.

E401: I don't see "we know better" as a good formatting rule. Auto-formatting is there to remove discussion about the formatting, and to be able to focus on what the code is doing instead. Again, perfectly fine to keep this as "too disruptive for the introduction of autopep8", but I feel this should be un-ignored on the not-too-long term.

E402: As for the import order & detecting reloads, I have found that the following way of doing things not only works, but also helps IDEs to understand what's going on:

import stdlib_module_a
import stdlib_module_b

import third_party_module_a
import third_party_module_b

import bpy

if 'local_module' not in locals():
    import local_module
else:
    import importlib
    local_module = importlib.reload(local_module)

When done like this, it follows PEP 8 - Imports. The advantage to IDEs comes from the fact that the import local_module happens before the reloading, so the reading order of the code matches the execution order (initial import happens before reloading); this avoids the IDE from complaining that the name is being accessed before it's even imported.

W690: If we are still "suffering" from lib2to3-generated code, it's time to get rid of the last bit of rotting corpse smell and just convert the code to Python 3.

About the last two rules, I feel that if import order is that sensitive, there is something wrong with the code. Solving that is of course not for autopep8, but to me it's a sign of code smell that would be good to get rid of eventually.

I'm fine with having this in the root directory.

If this is auto detected by editors, I guess this needs to go along with reformatting all the code in blender and add-ons repositories? Otherwise editing files without accidentally reformatting will be difficult.

And then if we enforce this, it should run as part of make format.

In D14686#394761, @Brecht Van Lommel (brecht) wrote:

If this is auto detected by editors, I guess this needs to go along with reformatting all the code in blender and add-ons repositories? Otherwise editing files without accidentally reformatting will be difficult.

Was partially done in
rB3035235defe0: Cleanup: run autopep8 on tests/
rB58d86527ae28: Cleanup: run autopep8 on release/scripts/startup/

And then if we enforce this, it should run as part of make format.

This diff will leave https://developer.blender.org/diffusion/BDT/browse/master/utils/autopep8_clean.py redundant (which is good) since configuration is in pyproject.toml and file discovery + multiprocessing is better done in https://developer.blender.org/diffusion/BDT/browse/master/utils_maintenance/clang_format_paths.py. Would need some renaming and rework though. Existing features like formatting only modified files can be reused for python files also.

Thanks for the feedback, some updates:

• Moved the script into the root directory and defined exclude so extern/ and some data-files for example are excluded.
• Added "Further work" section to the patch description.
• @Sybren A. Stüvel (sybren) in general I'm fine with removing some of the ignore items separately from this patch, as you mention. Although it depends how much benefit we gain from this too (if we have to disable autopep8 in many places then we might consider leaving them as-is). Note that this ignore list was copied from source/tools/utils/autopep8_clean.py script, so I had to refresh my memory on the details of why some of these ignored.
  • E401 shouldn't have been ignored, it's possible at the time of adding it was useful (2018), but now it's fine to remove (only caused a single change in Blender's code).
  • W690 at the time of writing the comment I just noticed it added an import, now - re-running this on all scripts (including addons), performs no changes besides adding a duplicate import, which is a bug AFAICS. lib2to3 is planned to be deprecated / removed from Python. Since we aren't writing Python2 style code that needs to be updated, I don't see much value in keeping the option enabled.
  • Regarding your other points, I rather evaluate them as separate patches.

In D14686#395321, @Brecht Van Lommel (brecht) wrote:

Adding autopep8 to the precompiled libs is straightforward (P2900). Should this wait until your pull request is merged in a released version?

If we don't wait, it means the exclude paths will be ignored by any IDE's that launch autopep8 with an absolute file path (checked and VSCode does this for e.g.),
however the exclusions are mainly important when running recursively on a directory which I'd assume isn't typically done from IDE's.
For recursive execution we can control how autopep8 is called via make format and my pull-request isn't needed.

In the case of add-ons it means the pyproject.toml will be used too when it shouldn't be.

So I'm not sure if this would cause problems for developers practice.

I guess there is no good reason to wait then: D14716: Build: add autopep8 to bundled Python packages