-
Notifications
You must be signed in to change notification settings - Fork 120
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
Introduce Ruff #821
Introduce Ruff #821
Conversation
Should we add some mention of ruff to the docs? Also, in ixmp4, we have an optional dependency group called "dev" in pyproject.toml and that includes a minimum version for ruff. Should we add something similar here? |
Please also add the badge to index.rst, should be sufficient.
Yes, please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks quite good overall, a few small comments in line. Maybe you could mention ruff
the PULL_REQUEST_TEMPLATE.md
or CONTRIBUTING.rst
pyproject.toml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so that I understand it correctly, we will now have a setup.cfg
, setup.py
and pyproject.toml
?
Could you combine everything into a pyproject.toml or move the ruff settings to setup.cfg or setup.py?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that you can also use a ruff.toml
or .ruff.toml
file (https://github.com/astral-sh/ruff?tab=readme-ov-file#configuration). I'd prefer that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, ruff won't read configuration from setup.cfg
or setup.py
. My intention with using pyproject.toml
was to next use #823 to migrate to poetry, which will read all settings from pyproject.toml
, so that way the files will be combined. Let me clean up this PR as per your suggestions and then create one with poetry and then we can decide: if you don't like poetry, I can migrate the settings here to ruff.toml
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point. I'd prefer though if you use ruff.toml
for now and then in a future PR integrate setup.cfg
and setup.py
into one pyproject.toml
.
You also have to use the ruff-badge in docs/index.rst on line 8. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thank you!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #821 +/- ##
=====================================
Coverage 94.8% 94.8%
=====================================
Files 64 64
Lines 6089 6092 +3
=====================================
+ Hits 5775 5778 +3
Misses 314 314 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to merge from my side if you change the pyproject.toml to ruff.toml
I can't merge it myself though, don't have permissions for that, so please go ahead. |
Please confirm that this PR has done the following:
[ ] Tests AddedBut updated black CI workflowDescription of PR
As suggested by #820 (comment), this PR cherry-picks the commits from #820 that refer to ruff and amends some of them that got mixed with Python mentions.