Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enforce code format with black + isort #1738

Merged
merged 15 commits into from
Jul 25, 2023

Conversation

dbungert
Copy link
Collaborator

@dbungert dbungert commented Jul 21, 2023

  • CI now requires that the code is already formatted with black and isort
  • local reformatting can be done with 'make format' or 'make black' or 'make isort'. tox.ini has been overhauled to remove the unusable environments (all of them) and add black and isort environments. tox and the workflow are using the same version selectors.
  • using tox also required bdist_wheel to be working, so also fix that
  • appropriate configs are in place so that the tools (black, isort, flake8) should be playing nice with each other
  • pre-commit hooks added, to incrementally run the format steps and fail the commit if not formatted

Suggestion for reviewing this PR:

  • the source directories are going to be filled with reformatted code. The bulk of the changes to set this up have been structured before the reformat commit, then the reformat is applied, then the workflow is enabled and .git-blame-ignore-revs is populated.
  • a few source directories have interesting changes that are not generated by the code formatting tools - these have a "fix lint" note in the commit summary, and these commits are before the reformat. These changes are about fixing things that black would otherwise reformat in a way that would trigger flake8 failure.
  • a supplemental commit to fix joining of long lines has been added, after the format commit, by temporarily using black --experimental-string-processing

Copy link
Member

@ogayot ogayot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings on this one. This is affecting the entire tree - and therefore will make all existing PRs obsolete. When I see some of the changes (e.g., https://github.com/canonical/subiquity/pull/1738/files#diff-dc9f10a528237b402741453879a9e45cc48cbc8a9d4ce289c096b41f649bb895), it doesn't necessarily feel like an improvement IMO. Can we somehow relax this by making only new code affected ?

If we are headed this direction, I would like the extra quoting in merged multi-line strings to be dropped, e.g.:

"Select one of available recovery systems and a desired " "action to execute."

If you search " " in the code, you will find plenty of occurrences.

@mwhudson
Copy link
Collaborator

As you know I'm in favour of this (I expect existing PRs can be un-obsoleted by running black on them first). I agree that the joined lines are strange and should be fixed.

@mwhudson
Copy link
Collaborator

Oh yeah, would it be possible to add something to CONTRIBUTING.md about how to add a pre-commit hook to enforce this? I guess I'll want something for emacs too but I'm sure I can figure that out on my own :-)

@dbungert
Copy link
Collaborator Author

If we are headed this direction, I would like the extra quoting in merged multi-line strings to be dropped, e.g.:

"Select one of available recovery systems and a desired " "action to execute."

Yes, those are pretty ugly.
The --experimental-string-processing option is around this problem. AIUI, that wouldn't be a mainline feature until v24 (unless it misses the window.)

@dbungert
Copy link
Collaborator Author

Marking draft as this requires a rebase before merge. It is otherwise reviewable.

@dbungert dbungert marked this pull request as draft July 24, 2023 16:22
@dbungert
Copy link
Collaborator Author

(I don't want to enable preview features generally, just using that for testing purposes)

@dbungert dbungert marked this pull request as ready for review July 24, 2023 20:31
@dbungert
Copy link
Collaborator Author

Oh yeah, would it be possible to add something to CONTRIBUTING.md about how to add a pre-commit hook to enforce this?

Thanks for the excuse to learn more about these - done!

@dbungert
Copy link
Collaborator Author

If we are headed this direction, I would like the extra quoting in merged multi-line strings to be dropped, e.g.:
"Select one of available recovery systems and a desired " "action to execute."

Yes, those are pretty ugly. The --experimental-string-processing option is around this problem. AIUI, that wouldn't be a mainline feature until v24 (unless it misses the window.)

These are now fixed in a follow-up format commit.

Copy link
Member

@ogayot ogayot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dbungert dbungert requested a review from mwhudson July 25, 2023 20:19
Copy link
Collaborator

@mwhudson mwhudson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this done

start a pyproject.toml file, as black will only read config from it.
All the existing environments were unused and broken. Remove them.
Add black and isort environments to help run a consistent version.
We've been using the distro flake8 for some time.  Also tox -e flake8 is
broken.  Just remove this note.  Also drop unneded trailing slashes.
black will reformat this in a manner that trips flake8.
black will reformat this in a manner that trips flake8.
black will reformat this in a manner that trips flake8.
@dbungert dbungert merged commit 2a8f1cf into canonical:main Jul 25, 2023
11 checks passed
@dbungert dbungert deleted the code-format-enforce branch July 25, 2023 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants