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.

Sign up for GitHub

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

Jump to bottom

Replace black with ruff and add pre-commit #110

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Replace black with ruff and add pre-commit #110

wants to merge 6 commits into from

Conversation

davidwilby
Copy link
Collaborator

Hey @tom-andersson et al - just a maintenance-related PR for your perusal, feel free to reject if this isn't your thing.

What this PR does

  • resolves Potential new feature: ruff linting #109
  • replaces black with ruff as the linter of choice, partially because of extra features such as linting docstrings in google-style (which black may do as well, but I haven't been able to find it)
  • adds pre-commit and a pre-commit config to run the ruff linter and formatter on a pre-commit hook.
  • adds info to CONTRIBUTING.md detailing the use of these tools.

What this PR doesn't do

  • actually formatting the codebase with ruff so far in this branch to avoid the massive diff
  • add a pull request template to remind contributors to follow these steps (that will follow in another PR)

At the moment the CI check fails since ruff is checking all the docstrings, we may be able to soften the docstring rules (configured in pyproject.toml) to something we agree with (e.g. I don't necessarily think that every module needs a docstring)

FYI @nilsleh

@davidwilby davidwilby mentioned this pull request Apr 26, 2024
Open
kallewesterling
kallewesterling approved these changes May 17, 2024
View reviewed changes
Copy link
Collaborator

@kallewesterling kallewesterling left a comment

Choose a reason for hiding this comment

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

LGTM!

@tom-andersson
Copy link
Collaborator

Thank you @davidwilby, this does look quite useful. I checked the error message of the CI and we now get many failing docstring checks. Can you 1) address the simpler syntax ones 2) silence more complex issues like missing public function docstrings, so that tests pass?

It looks like we can ignore error messages in pyproject.toml with:

[tool.ruff.lint]

ignore = [
    # Relax the convention by _not_ requiring documentation for every function parameter.
    "D417",
]

@davidwilby
Copy link
Collaborator Author

@tom-andersson I've added a number of rules to ignore and performed the remaining linting of docstrings (the code itself was completely compliant from using black)

The remaining linting failures are for missing docstrings on public methods/functions:

deepsensor/data/loader.py:809:9: D102 Missing docstring in public method
deepsensor/data/task.py:32:9: D102 Missing docstring in public method
deepsensor/model/convnp.py:446:9: D102 Missing docstring in public method
deepsensor/model/convnp.py:469:9: D102 Missing docstring in public method
deepsensor/model/convnp.py:492:9: D102 Missing docstring in public method
deepsensor/model/convnp.py:517:9: D102 Missing docstring in public method
deepsensor/model/convnp.py:553:9: D102 Missing docstring in public method
deepsensor/model/convnp.py:588:9: D102 Missing docstring in public method
deepsensor/model/convnp.py:620:9: D102 Missing docstring in public method
deepsensor/model/convnp.py:638:9: D102 Missing docstring in public method
deepsensor/model/convnp.py:977:5: D103 Missing docstring in public function
deepsensor/model/model.py:84:9: D102 Missing docstring in public method
deepsensor/model/model.py:612:5: D103 Missing docstring in public function
deepsensor/tensorflow/__init__.py:19:5: D103 Missing docstring in public function
deepsensor/torch/__init__.py:19:5: D103 Missing docstring in public function
deepsensor/train/train.py:168:9: D102 Missing docstring in public method

I've had a go at adding a few missing docstrings in 678602a but other than that I don't know enough to write them.

One option (which I've used in a couple of places) is to add a # noqa tag for functions/methods that aren't really meant for public usage.

Could you add some docstrings or advise on those that are remaining when you get the chance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kallewesterling kallewesterling kallewesterling approved these changes

@tom-andersson tom-andersson Awaiting requested review from tom-andersson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Potential new feature: ruff linting
3 participants
@davidwilby @tom-andersson @kallewesterling