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

Linting and formatting with Ruff #56

Merged
merged 7 commits into from
Sep 11, 2024
Merged

Conversation

akx
Copy link
Contributor

@akx akx commented Jul 25, 2024

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
Copy link
Contributor

eskech commented Jul 30, 2024

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
Copy link
Contributor Author

akx commented Aug 26, 2024

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

@Chroxvi
Copy link
Contributor

Chroxvi commented Aug 27, 2024

@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
Copy link
Contributor Author

akx commented Aug 27, 2024

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: 96f63ab

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

@Chroxvi Chroxvi self-assigned this Aug 27, 2024
@Chroxvi Chroxvi added the enhancement New feature or request label Aug 27, 2024
@akx
Copy link
Contributor Author

akx commented Aug 27, 2024

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

@akx akx changed the title Linting and typo fixes Linting and formatting with Ruff Aug 27, 2024
@Chroxvi Chroxvi mentioned this pull request Sep 2, 2024
@akx akx force-pushed the lint-and-ci-and-so-on branch 2 times, most recently from d27d179 to 35c9614 Compare September 3, 2024 08:05
@akx
Copy link
Contributor Author

akx commented Sep 3, 2024

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

@Chroxvi
Copy link
Contributor

Chroxvi commented Sep 3, 2024

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

Copy link
Contributor

@Chroxvi Chroxvi left a comment

Choose a reason for hiding this comment

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

So I finally got around to digging into this PR. I still very much the idea of introducing Ruff for linting and formatting!

Having looked more into it, I am a bit more hesitant to also introduce pre-commit - see my specific review comment about this. @akx, I would like your opinion on this.

I have suggested an extended list of Ruff rules to enable based on the linting done by my editor - this may be too many rules to enable for now.

The rest is just a bunch of minor comments and suggestions for improvements.

Finally, when we get to #57, I think we should move the ruff.toml config to pyproject.toml since we are already collecting all other configuration in pyproject.toml. For now, though, lets use the ruff.toml config file.

.github/workflows/CI_pull_request.yml Outdated Show resolved Hide resolved
.github/workflows/CI_pull_request.yml Outdated Show resolved Hide resolved
.github/workflows/CI_pull_request.yml Show resolved Hide resolved
cotainr/cli.py Show resolved Hide resolved
cotainr/tests/tracing/test_log_dispatcher.py Outdated Show resolved Hide resolved
ruff.toml Outdated Show resolved Hide resolved
ruff.toml Show resolved Hide resolved
ruff.toml Show resolved Hide resolved
ruff.toml Outdated Show resolved Hide resolved
ruff.toml Show resolved Hide resolved
@akx akx force-pushed the lint-and-ci-and-so-on branch 3 times, most recently from 769eed3 to 533949f Compare September 6, 2024 11:14
@Chroxvi
Copy link
Contributor

Chroxvi commented Sep 9, 2024

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

@Chroxvi Chroxvi mentioned this pull request Sep 9, 2024
@akx
Copy link
Contributor Author

akx commented Sep 10, 2024

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 #56 (comment)

@akx akx force-pushed the lint-and-ci-and-so-on branch 2 times, most recently from b88ad5f to 684bf38 Compare September 10, 2024 16:47
@Chroxvi
Copy link
Contributor

Chroxvi commented Sep 11, 2024

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.

Copy link
Contributor

@Chroxvi Chroxvi left a comment

Choose a reason for hiding this comment

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

Approved!

@Chroxvi Chroxvi merged commit 9e66472 into DeiC-HPC:main Sep 11, 2024
14 checks passed
@akx akx deleted the lint-and-ci-and-so-on branch September 11, 2024 05:46
@Chroxvi Chroxvi mentioned this pull request Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants