-
Notifications
You must be signed in to change notification settings - Fork 8
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
chore(ruff): configure ruff #33
Conversation
6d772c9
to
ac13516
Compare
.github/workflows/python.yml
Outdated
runs-on: ubuntu-20.04 | ||
strategy: | ||
matrix: | ||
python-version: [ "3.9", "3.10", "3.11", "3.12" ] |
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.
Is it necessary to run this job for so many versions of python? Especially if the goal is to simply to run ruff - then presumably one should be sufficient?
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.
It's not just to run ruff, but rather configure CI as a whole and add ruff
linting on top of that. I added clarification to the issue description.
But to the point of multiple Python versions, well, it could be an overkill, this is essentially a CLI utility and not an actual library
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.
Saw this before #30 .
I think it could be good split ruff and the build into separate actions - it'll be clearer, simpler.
If joint, ruff would be run for each python version, as opposed to separating it out and simply running the ruff action with one python version.
|
||
name = "GitHub Registry Source" | ||
is_primary = True | ||
|
||
def __init__(self, domain='mainnet'): | ||
def __init__(self, domain="mainnet"): | ||
self.domain = domain | ||
|
||
def get_publication_endpoint(self) -> str: |
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.
@KPrasch FYI another use of the development
branch for contract registry information. Related to nucypher/nucypher#3336.
.github/workflows/python.yml
Outdated
pip install setuptools wheel ruff | ||
|
||
- name: Run ruff | ||
run: ruff check nucypher_ops |
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.
Looking at nucypher
's use of ruff (see https://github.com/nucypher/nucypher/blob/main/.github/workflows/ruff.yaml), should this be?
ruff --output-format=github nucypher_ops
Just wondering if there is a reason for the discrepancy.
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.
No reason, I don't know what --output-format=github
does.
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 believe (?) the format caters to specific output that can be viewed as gh actions workflow outputs. Eg. https://github.com/nucypher/nucypher/actions/runs/6708762664/job/18230283150
a9de1cb
to
1a5f3a0
Compare
- name: Build dist | ||
run: python setup.py sdist bdist_wheel | ||
|
||
lint: |
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.
👌
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.
🎸
[tool.ruff] | ||
exclude = ["nucypher_ops/__init__.py"] # false positives | ||
select = ["E", "F", "I"] | ||
line-length = 180 |
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.
Pretty long line length 😅 - although it's hard to specify a value that everyone agrees on hehe.
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 prefer shorter lines myself, but it would be very much out of style with the current code style in nucypher-ops
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.
👍. Another solution is to ignore line length errors (https://docs.astral.sh/ruff/rules/line-too-long/) i.e. add this to the [tool.ruff] section:
ignore = ["E501"]
then you can remove the line-length
setting. Up to you though.
1a5f3a0
to
da64c63
Compare
Co-authored-by: Derek Pierre <derek.pierre@gmail.com> Update .github/workflows/python.yml Co-authored-by: Derek Pierre <derek.pierre@gmail.com> Update nucypher_ops/__about__.py Co-authored-by: Derek Pierre <derek.pierre@gmail.com> Update nucypher_ops/__about__.py Co-authored-by: Derek Pierre <derek.pierre@gmail.com>
da64c63
to
ed3eb89
Compare
<<<<<<< HEAD | ||
======= | ||
|
||
>>>>>>> cced0c2 (Update .github/workflows/python.yml) |
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.
@piotr-roslaniec did this get merged with conflicts?
Required reviews: 1
Based on: #32
nucypher/nucypher
ruff
for linting, fixes lint errors