Switch to ruff #543
Switch to ruff #543
Conversation
Adjust format of pre-commit config to more conventional YAML style
IMPORTANT: Ruff isn't added to requirements.txt because it's run through pre-commit
|
I will automatically update this comment whenever this PR is modified
|
| # bare except | ||
| "E722", | ||
| # unable to detect undefined names | ||
| "F403", |
There was a problem hiding this comment.
Some of these are pretty easy to polish off. F403 is one -- here we import * in an __init__.py. Instead, we should import the objects we want to expose by name. Any thoughts on what objects we want to expose here?
https://github.com/icesat2py/icepyx/blob/development/icepyx/quest/dataset_scripts/__init__.py
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #543 +/- ##
============================================
Coverage 66.05% 66.05%
============================================
Files 36 36
Lines 3049 3049
Branches 537 537
============================================
Hits 2014 2014
Misses 947 947
Partials 88 88 ☔ View full report in Codecov by Sentry. |
| # Use the Ruff linter to annotate code style / best-practice issues | ||
| # NOTE: More config provided in pyproject.toml | ||
| - name: Lint and annotate PR | ||
| uses: chartboost/ruff-action@v1 |
There was a problem hiding this comment.
Looks like this GitHub Action isn't compatible with ruff>=0.5.0 (ChartBoost/ruff-action#30) and there's also a separate issue that they're looking into transferring this to another organization (which might take a while).
So... could we just use pre-commit.ci instead? Or are the annotations good to have?
There was a problem hiding this comment.
I'm cool with just using pre-commit.ci. I was trying to avoid any regression in functionality. I personally don't find annotation of PRs to be that valuable, but I imagine the feature would be more valuable to someone who doesn't know how to use the linter at the CLI. 🤔
It looks to me from reading the issue like this isn't an incompatibility issue yet though? Or has Ruff gone forward with the breaking change? EDIT: Nevermind, we can see right here that it's broken :)
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed linting on this PR. I was using an outdated argument form!
There was a problem hiding this comment.
I personally don't find annotation of PRs to be that valuable, but I imagine the feature would be more valuable to someone who doesn't know how to use the linter at the CLI.
You've identified exactly why we have the PR annotations set up. Precommit is an additional barrier to new contributors, so I wanted to be able to enforce the linting and formatting without absolutely requiring they complete yet another setup step to contribute.
| # GOAL: | ||
| # syntax check doctests in docstrings | ||
| # doctests = True |
There was a problem hiding this comment.
No way to syntax check docstrings in ruff yet - astral-sh/ruff#3542. But we can at least format the docstrings, see below.
There was a problem hiding this comment.
Are we executing doctests? That'll give us the syntax check!
There was a problem hiding this comment.
No, not yet, but can do that later!
Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Wei Ji <[email protected]>
| black | ||
| flake8 |
There was a problem hiding this comment.
Suggest we add ruff here (unless it only ever runs on CI?)
There was a problem hiding this comment.
It runs in GitHub Actions and in pre-commit.ci, but seems useful to also have it in the dev requirements!
@JessicaS11 and I had a chat about this on the phone today. I feel Ruff's capacity to automatically fix so much stuff is a game-changer! I have to think about linting much less.