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

python: Update black to 24.4.0 #3545

Merged
merged 11 commits into from
Apr 23, 2024
Merged

python: Update black to 24.4.0 #3545

merged 11 commits into from
Apr 23, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Mar 30, 2024

Updates black to the latest version, in the GitHub Actions workflows and pre-commit config.
Includes formatting for Jupyter notebooks too, since it is supported since version 21.8b0.

The specific changes in the configuration are made in different commits:

  • Require version (pyproject.toml): Black allows setting a version, and will error out if not the correct one used. Since it accepts a more general version (rather than the full version), and that Black mentions it follows their stability policy, setting "24" will ensure all users are formatting with a version that will output the same style. It should solve the cases where contributors weren't using the same black version as was required in CI.
  • Target version (pyproject.toml): Black can make some choices when it knows all python versions that the project should support. It is now set for supporting Python 3.8 through Python 3.12 (removed Python 3.7)
  • Extend-exclude (pyproject.toml): Instead of setting exclusion patterns explicitly, keep black's good defaults, and only add python/libgrass_interface_generator/. In addition of common exclusions, black excludes all paths listed in .gitignore. The way the setting was made looks like an older suggested config. We are now aligned with the current recommendations.
  • Include (pyproject.toml): The include pattern was overriding black's default include pattern, that was changed since the config file was written, thus Jupyter notebooks weren't formatted even if the feature was installed and available, and pre-commit was fixing them. Now, we only use the default include pattern (deleting the config line).

The files are formatted with the new version in the last commits, the next-to-last commit is for all files in the repo (171 files changed), and the last commit for the 9 Jupyter notebooks that had changes. I separated them for easier review (for looking at only one file at a time).

@echoix echoix added this to the 8.4.0 milestone Mar 30, 2024
@github-actions github-actions bot added GUI wxGUI related CI Continuous integration vector Related to vector data processing raster Related to raster data processing temporal Related to temporal data processing Python Related code is in Python libraries module docs general imagery tests Related to Test Suite raster3d notebook labels Mar 30, 2024
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@echoix
Copy link
Member Author

echoix commented Mar 30, 2024

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

This is because the name of the configuration includes all the matrix variables. We could eventually rework the workflow to not have this every time a version changes, and not have a new "configuration" that will pile up (in the second screenshot) and be orphaned.
image
image

@echoix
Copy link
Member Author

echoix commented Mar 30, 2024

Most of the changes are adding a blank line between the file's docstring and the code, or removing an extra blank between it. There's only like a dozen or so "real" changes in the .py files.

@echoix
Copy link
Member Author

echoix commented Mar 30, 2024

I continued and made some tests using ruff format basing myself from these changes. Unfortunately, there was some files changed between both, in multiple ways.

Versions:
black: 24.3.0, with jupyter feature
ruff format: 0.3.4

Contents of this PR (black 24.3.0) + ruff format: 79 changed files. (includes libgrass_interface_generator)
Contents of this PR (black 24.3.0) + ruff format --target-version py38: 55 changed files. (excludes libgrass_interface_generator)
but most importantly,
Contents of this PR (black 24.3.0) + ruff format + black 24.3.0: 79+26 changed files (26 more, total 54 files (so some are reverting themselves)). (includes libgrass_interface_generator)
Contents of this PR (black 24.3.0) + ruff format --target-version py38 + black 24.3.0: 55+26 changed files (26 more, total 30 files (so some are reverting themselves)) (excludes libgrass_interface_generator)

Is the black preview style better?
Contents of this PR (black 24.3.0) + ruff format + black 24.3.0 --preview: 79+31 changed files (31 more). (includes libgrass_interface_generator)
Contents of this PR (black 24.3.0) + black 24.3.0 --preview: 24 changed files
Contents of this PR (black 24.3.0) + black 24.3.0 --preview + ruff format: 24+61 changed files (61 more) (includes libgrass_interface_generator)
Contents of this PR (black 24.3.0) + black 24.3.0 --preview + ruff format . --target-version py38: 24+61 changed files (61 more) (includes libgrass_interface_generator)
Contents of this PR (black 24.3.0) + ruff format . --target-version py38: 24+61 changed files (61 more) (includes libgrass_interface_generator)

Ruff preview style has way more changes, I didn't bother writing the results down.

Some of these changes could be solved once for both formatters, since some of the differences is some missing/extra blank lines around some comments/docstring at the beginning of the files that black 24 didn't fix, or some tabulations in the copyright headers changed to spaces. Most of the other differences are because of comments at the end of a line instead of before a line, which forces unnatural parenthesis to have the comment at the end of the last line. Sometimes I find that ruff made better choices, sometimes black. When the comment at the end of the line isn't a linter instruction (like # pylint: disable=W0612), then it would be possible to move the comment above and probably have both agree.

However, there are still some differences that ruff makes the same choices that were made with the black version before this PR.
So, in any case, when we would be ready to use the ruff as a formatter (ruff as a linter doesn't matter here, ruff does ruff check . and ruff format .), we would need to decide on which tool to use. Ruff is faster.

@echoix
Copy link
Member Author

echoix commented Apr 13, 2024

Have to update the PR, applying the changes, since it was waiting too long and the repo had other PRs merged meanwhile

@echoix echoix changed the title python: Update black to 24.3.0 python: Update black to 24.4.0 Apr 14, 2024
@echoix
Copy link
Member Author

echoix commented Apr 14, 2024

Updated the new files and set the black version to the 24.4.0 release

@echoix echoix force-pushed the update-black-24.3.0 branch 2 times, most recently from ad01df4 to 0fa593a Compare April 18, 2024 11:33
@wenzeslaus wenzeslaus added the add to git-blame-ignore-revs Once merged, add to .git-blame-ignore-revs label Apr 18, 2024
wenzeslaus
wenzeslaus previously approved these changes Apr 18, 2024
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Thanks for the overall update. The config was indeed dated since it was from the beta version times. Yes, notebooks should be formatted too.

The formatting changes indeed improve the formatting in most cases with couple cases being a neutral change for those specific lines. I added the add ref ignore label.

@echoix
Copy link
Member Author

echoix commented Apr 18, 2024

I see that the rebase this morning brought some other changes that happened since the weekend. I'll need to repost the changed files and will need a new approval

@echoix
Copy link
Member Author

echoix commented Apr 18, 2024

@wenzeslaus Can you reapprove after the single change in wxplot/profile.py change?

@echoix echoix enabled auto-merge (squash) April 18, 2024 22:37
@echoix
Copy link
Member Author

echoix commented Apr 18, 2024

Once merged, PRs that change python code should be brought up to date in order to not have the workflow fail on main, and have the suggestions shown in the PR (if possible).

@echoix
Copy link
Member Author

echoix commented Apr 19, 2024

The PRs merged yesterday after my rebase didn't have problems with black 24.4 finally.

@echoix
Copy link
Member Author

echoix commented Apr 23, 2024

Can we regive it a go? it was approved 5 days ago on a failing CI check, that is fixed and kept updated.

@echoix echoix merged commit f8115df into OSGeo:main Apr 23, 2024
27 checks passed
@echoix echoix deleted the update-black-24.3.0 branch April 23, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to git-blame-ignore-revs Once merged, add to .git-blame-ignore-revs CI Continuous integration docs general GUI wxGUI related imagery libraries module notebook Python Related code is in Python raster Related to raster data processing raster3d temporal Related to temporal data processing tests Related to Test Suite vector Related to vector data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants