search

DeiC-HPC / cotainr

cotainr - a user space Apptainer/Singularity container builder.
European Union Public License 1.2
21 stars 5 forks source link

Linting and formatting with Ruff #56

Closed akx closed 2 months ago

akx commented 4 months ago

This PR enables linting using Ruff via pre-commit (which also runs these lints in CI).

The ruff.toml configuration file is rather conservative at this point; more lint families can be added as required. The line-length of 88 characters was chosen by checking which common line-length causes the shortest ruff format delta.

eskech commented 4 months ago

Just a short comment. @Chroxvi is on vacation and is back in two weeks. I really want him to do the review. Sorry for letting you wait until he is back.

akx commented 3 months ago

Gentle nudge (as I too am back from vacation) :)

Chroxvi commented 3 months ago

@akx Thanks a lot for this contribution - and sorry for not getting back to you any sooner.

A pre-commit linting setup for cotainr has been on my wishlist for quite some time, so let's get this rolling!

The cotainr style guide states that we use black for code formatting, numpydoc for formatting docstrings, and in general adhere to PEP8. In addition to that I have locally been using isort, flake8 and mypy (though mypy doesn't make much sense before we add type annotations to cotainr).

I don't have a strong opinion on any particular tool, so we can introduce ruff, if you thing that is the best suitable tool for such a pre-commit linting setup. Given the high level of compatibility in formatting between ruff and black, I think it is acceptable to switch to ruff for formatting. Would you agree that ruff is a viable solution to replace my setup with black, isort, flake8, and mypy? And do we need any additional configuration of ruff to better align with what has been the implicit standards in cotainr so far? My only concern here is to introduce too many code changes that are only related to formatting since it kind of destroys the git history.

akx commented 3 months ago

I don't have a strong opinion on any particular tool, so we can introduce ruff, if you thing that is the best suitable tool for such a pre-commit linting setup. [...] Would you agree that ruff is a viable solution to replace my setup with black, isort, flake8, and mypy?

Ruff doesn't do actual type-checking, so it doesn't replace mypy. It can do some rudimentary annotation checks, but not actual analysis.

For the other tools, yes (disclaimer: I'm an early minor contributor to Ruff). It's a lot faster than the other tools combined (so it stays out of your way, as it were, and is more eco friendly too) :)

And do we need any additional configuration of ruff to better align with what has been the implicit standards in cotainr so far?

More rules could be enabled in a follow-up PR to make things stricter. Ruff can also be configured to enforce numpydoc style for docstrings.

My only concern here is to introduce too many code changes that are only related to formatting since it kind of destroys the git history.

Since ruff-format and Black are practically the same thing (aside from ruff-format doing a better job (IMO) in certain corner cases), I don't think that's an issue here.

The auto-format commit itself is quite short: https://github.com/DeiC-HPC/cotainr/pull/56/commits/96f63abec7f5f2ffa703cc93af5d3855c20cd755

This PR could be merged with a regular merge commit, and that commit added to .git-blame-ignore-revs, if need be.

akx commented 3 months ago

I should probably split the typo fixes out of this PR, for clarity :)

akx commented 2 months ago

@Chroxvi Rebased post #59 merge. Anything I can do to move this (and my other PRs) forward?

Chroxvi commented 2 months ago

@akx Excellent, that helps a lot! At this point, I think it is entirely up to me move this forward. Sorry for not getting back to you sooner. I hope to be able to move this forward this week.

Chroxvi commented 2 months ago

@akx I think the main missing part here is handling my comments related to adding more details about this linting setup to the documentation. Would you be willing to add these? If not, I am also very happy to merge this PR once the details in the other few remaining comments have been sorted out.

akx commented 2 months ago

I think the main missing part here is handling my comments related to adding more details about this linting setup to the documentation. Would you be willing to add these?

Done deal. To keep this more reviewable, I can follow this up after merging with another PR that adds more selects as discussed in https://github.com/DeiC-HPC/cotainr/pull/56#discussion_r1750032345

Chroxvi commented 2 months ago

Excellent! I'll merge this now.

@akx Thanks for a lot for hanging in here and for all the valuable discussions around best practices for using Ruff, pre-commit, and GitHub actions.