adafruit / cookiecutter-adafruit-circuitpython

Cookiecutter template for Adafruit's CircuitPython libraries.
MIT License
22 stars 37 forks source link

Add isort, update black, sort and update global pylint disables #236

Closed justmobilize closed 4 months ago

justmobilize commented 6 months ago

Add isort Update black Sort and update global pylint disables

justmobilize commented 6 months ago

This has updated changes to https://github.com/adafruit/cookiecutter-adafruit-circuitpython/pull/224

justmobilize commented 6 months ago

I am happy to help update any repos if this is merged

FoamyGuy commented 6 months ago

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

justmobilize commented 6 months ago

@FoamyGuy I will guess it's all over the place. But here's a concrete example of when I added it to requests: https://github.com/adafruit/Adafruit_CircuitPython_Requests/pull/151/commits/9e7e701d22ccfa879b94626257450703cca8403a

justmobilize commented 6 months ago

I can also open a few random drafts tomorrow if it would help

FoamyGuy commented 6 months ago

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.

FoamyGuy commented 6 months ago

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.

justmobilize commented 6 months ago

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.

dhalbert commented 6 months ago

MicroPython has adopted ruff as a replacement for black and pylint, which we might consider in the long run, though it's a pervasive change to switch. I have seen other projects switch to ruff as well.

Discussion of its import-sorting behavior: https://github.com/astral-sh/ruff/issues/8926

justmobilize commented 6 months ago

If you were to switch to ruff, this would be a step in the right direction, yes?