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

Use ruff to format and lint #114

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Use ruff to format and lint #114

wants to merge 8 commits into from

Conversation

Huite
Copy link
Contributor

@Huite Huite commented Jun 18, 2024

In a nutshell:

  • I've replaced the black and isort sections in pyproject by a ruff section.
  • I've selected a number of rules. Mostly based on what I use in other repositories. They could probably be expanded further, but these already lead to quite some improvements.
  • Most of the ignores are missing docstrings. These should be added at some point, I think, and the ignores can be removed.
  • I'm excluding the examples, because there are a number of utilities defined there. I think this example should be moved somewhere else anyway?
  • I've updated the CI workflow, although it just runs ruff check . now. Not sure if this replicates the current behavior (or how far that is possible with ruff).

There's some funny stuff here and there, which is touched by ruff, but not everything is fixed.
For example, this bit in stripareasink changetrange:

            xn = x1 + u * (x2 - x1)
            yn = y1 + u * (y2 - y1)
            zn = xyzt1[2] + u * (xyzt2[2] - xyzt1[2])

None of these are used...?
ruff has removed the assignment, but I think the lines could be deleted outright.

@Huite
Copy link
Contributor Author

Huite commented Jun 18, 2024

I'm not sure why the coverage report is way down, but I've remember seeing sudden changes in coverage reports before (e.g. flopy).

I don't think the coverage is super meaningful at the moment, though, since most of the tests don't check the answers. It's still nice to know there's no syntax errors though -- although I think ruff should catch that now.

pyproject.toml Outdated Show resolved Hide resolved
timml/stripareasink.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@dbrakenhoff dbrakenhoff left a comment

Choose a reason for hiding this comment

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

Looks good, approving in advance. I had a few comments that might or might not be relevant.

@dbrakenhoff
Copy link
Collaborator

One more thought, we could consider adding isort and formatting checks using ruff, instead of only checking linting?

@Huite
Copy link
Contributor Author

Huite commented Jun 18, 2024

One more thought, we could consider adding isort and formatting checks using ruff, instead of only checking linting?

Running ruff check takes care of this.

If I scramble the imports:

import timml as tml
import numpy as np
import matplotlib.pyplot as plt

Ruff will complain appropriately:

❯ ruff check
examples\workshop_nhv\vlaketunnel_functions.py:3:1: I001 [*] Import block is un-sorted or un-formatted
Found 1 error.
[*] 1 fixable with the `--fix` option.

It's part of the current configuration:

[tool.ruff.lint]
# See: https://docs.astral.sh/ruff/rules/
select = [
    "C4",  # flake8-comprehensions
    "E",  # pycodestyle
    "F",  # pyflakes
    "I",  # isort
    "PT",  # pytest-style
    "D",  # pydocstyle
    "B",  # flake8-bugbear
    "NPY",  # numpy
]

@dbrakenhoff
Copy link
Collaborator

Sorry, i forgot import sorting is checked by ruff check. A code formatting check (a la black) can be added with ruff format --check? Or does ruff check do that as well?

@Huite
Copy link
Contributor Author

Huite commented Jun 19, 2024

You're right, turns out I was a little confused...

isort is part of the linting, but the formatting isn't. To check for the formatting, we need to run ruf format --check explicitly. I will add it to the CI.

@Huite
Copy link
Contributor Author

Huite commented Jun 19, 2024

Well, I just got confirmation from the CI that it does check:

All checks passed!
Would reformat: examples/timml_notebook0_sol.py
Would reformat: examples/workshop_nhv/vlaketunnel_functions.py
2 files would be reformatted, 31 files already formatted
Error: Process completed with exit code 1.

Turns out I had the ignore examples bit still in locally...

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.

None yet

2 participants