Add isort, update black, sort and update global pylint disables #236
Conversation
| @@ -4,9 +4,14 @@ | |||
|
|
|||
| repos: | |||
| - repo: https://github.com/python/black | |||
| rev: 23.3.0 | |||
| rev: 24.2.0 | |||
| hooks: | ||
| - id: black | ||
| - repo: https://github.com/PyCQA/isort |
| @@ -24,19 +29,19 @@ repos: | |||
| name: pylint (library code) | |||
| types: [python] | |||
| args: | |||
| - --disable=consider-using-f-string | |||
| - --disable=consider-using-f-string,wrong-import-order | |||
There was a problem hiding this comment.
Add wrong-import-order (isort)
| exclude: "^(docs/|examples/|tests/|setup.py$)" | ||
| - id: pylint | ||
| name: pylint (example code) | ||
| description: Run pylint rules on "examples/*.py" files | ||
| types: [python] | ||
| files: "^examples/" | ||
| args: | ||
| - --disable=missing-docstring,invalid-name,consider-using-f-string,duplicate-code | ||
| - --disable=consider-using-f-string,duplicate-code,invalid-name,missing-docstring,wrong-import-order |
There was a problem hiding this comment.
sort and add wrong-import-order (isort)
| - id: pylint | ||
| name: pylint (test code) | ||
| description: Run pylint rules on "tests/*.py" files | ||
| types: [python] | ||
| files: "^tests/" | ||
| args: | ||
| - --disable=missing-docstring,consider-using-f-string,duplicate-code | ||
| - --disable=consider-using-f-string,duplicate-code,invalid-name,missing-docstring,protected-access,wrong-import-order |
There was a problem hiding this comment.
sort and add invalid-name (tests have long names), protected-access (tests mess with protected things) and wrong-import-order (isort)
|
This has updated changes to #224 |
|
I am happy to help update any repos if this is merged |
|
@justmobilize in order to gauge how big of a change it will cause in the libraries can you make these changes in a branch in one library and open a draft PR with it? When it comes time to actually update them all we'll automate it, but it'd be nice to have a rough idea of how big or small of a change in the code this shift in the pre-commit configs will lead to. |
|
@FoamyGuy I will guess it's all over the place. But here's a concrete example of when I added it to |
|
I can also open a few random drafts tomorrow if it would help |
|
I'm not sure how we would automate the resulting changes. I was not involved as heavily when we first implemented Black code formatting, but I suspect we will need to use some technique similar to when that was done if we automated it. To the best of my knowledge the Adabot automation tools will only help with half of this effort, changing the .pre-commit config and other related config files. But it won't make the changes that the new pre-commit wants to enforce due to it's changed configs. We need something or someone to run pre-commit after these config changes are applied and then commit and push the results + follow up and ensure that it's passing actions / resolve anything preventing it from passing. I'm not sure if we have a different utility that can handle that part but it would be a rather large effort if it does have to be done partially manually due to the sheer number of libraries. Honestly I'm not sure I see much of the upside in isort. I don't really find the unsorted imports to be problematic, and pylint is already trying to enforce a certain sort order on the code that we'd be changing to instead sort via a new 3rd party dependency. I don't really find myself looking through lists of imports basically ever so the order they are in doesn't really ever make any difference to me. I don't really envision them being sorted in any other particular order being an improvement over the pylint enforced order they're in today, or any other possible order that they could have landed in truthfully. |
|
I've asked in the discord about when we adoted pylint and black initially to see if there is some other utility that was used back then that I am unaware of. If we do make these changes I'd love to have our plan ready to go beforehand of how we're going to get the libraries updated with the changes it brings. And ideally I'd like it if we can minimize the manual steps to whatever extent is possible. |
|
I hear you. Pylint will do some basic sorting, but doesn't care about structure. You can move things around and pylint won't care. isort will handle grouping and sorting, so like black the code is always the same. If people don't want this, I'm happy to leave it. If they do, I'm happy to run through all the repos. |
|
MicroPython has adopted Discussion of its import-sorting behavior: astral-sh/ruff#8926 |
|
If you were to switch to ruff, this would be a step in the right direction, yes? |
Add isort
Update black
Sort and update global pylint disables