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

Improve current ruff rules #1720

Merged
merged 4 commits into from
Jan 16, 2024
Merged

Conversation

juanitorduz
Copy link
Contributor

@juanitorduz juanitorduz commented Jan 15, 2024

I keep improving the changes introduced in #1700. Note that in the setup.cfg file we had max-line-length = 120. This PR updates this condition and adds all the remaining pyflake rules. Note this is very similar to pyro's config https://github.com/pyro-ppl/pyro/blob/dev/pyproject.toml

@juanitorduz
Copy link
Contributor Author

juanitorduz commented Jan 15, 2024

The tests are not failing because of ruff but because of the docs:

make -C docs html
make[1]: Entering directory '/home/runner/work/numpyro/numpyro/docs'
Running Sphinx v4.5.0
/opt/hostedtoolcache/Python/3.9.1[8](https://github.com/pyro-ppl/numpyro/actions/runs/7531465780/job/20500091407?pr=1720#step:6:9)/x64/lib/python3.[9](https://github.com/pyro-ppl/numpyro/actions/runs/7531465780/job/20500091407?pr=1720#step:6:10)/site-packages/nbsphinx.py:1450: RuntimeWarning: You are using an unsupported version of pandoc (2.9.2.1).
Your version must be at least (2.14.2) but less than (4.0.0).
Refer to https://pandoc.org/installing.html.
Continuing with doubts...
  nbconvert.utils.pandoc.check_pandoc_version()
loading translations [en]... done

Sphinx version error:
The sphinxcontrib.applehelp extension used by this project needs at least Sphinx v5.0; it therefore cannot be built with this version.
make[1]: *** [Makefile:20: html] Error 2
make[1]: Leaving directory '/home/runner/work/numpyro/numpyro/docs'
make: *** [Makefile:26: docs] Error 2
Error: Process completed with exit code 2.

@juanitorduz
Copy link
Contributor Author

juanitorduz commented Jan 15, 2024

A suggested fix for the docs: #1721 Once this one is merged (or any other solution). I will rebase this one, and it should be 🟢!

pyproject.toml Outdated
@@ -31,13 +31,11 @@ exclude = [
]

# Same as Black.
line-length = 88
line-length = 120
Copy link
Member

Choose a reason for hiding this comment

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

I guess we prefer 88 here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think before we started the migration to ruff we had 120 (see original PR).
I can of course change it to 88. I might need to format some code a bit if I remember correctly. Is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I set 88 I get Found 821 errors (this comes from the 120 rule I mentioned). I could either keep 120 or set it to 88 and ignore the E501 rule as it is very hard to fix 821 errors :)

Copy link
Member

Choose a reason for hiding this comment

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

Previously, we allowed doc length 120 and code length 88. Do you think that we can maintain that behavior? If we set 120 here, will long code be formatted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we set 120 there is not refactor needed at all. I guess we can simply add 88 and ignore the rule for docstrings

Copy link
Member

Choose a reason for hiding this comment

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

Seems like

[tool.ruff.lint.pycodestyle]
max-line-length = 120

does the job

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed! Thanks 💪 ! See 493da8e

Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

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

Thanks, @juanitorduz!

@fehiepsi fehiepsi merged commit f6d118d into pyro-ppl:master Jan 16, 2024
4 checks passed
@juanitorduz juanitorduz deleted the complete_ruff_rules branch April 30, 2024 19:03
OlaRonning pushed a commit to aleatory-science/numpyro that referenced this pull request May 6, 2024
* improve current ruff rules

* rm comments

* fix lengths

* trim
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