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

Ruff linter #7091

Merged
merged 22 commits into from
Jan 11, 2024
Merged

Ruff linter #7091

merged 22 commits into from
Jan 11, 2024

Conversation

juanitorduz
Copy link
Contributor

@juanitorduz juanitorduz commented Jan 8, 2024

Closes #7084

I tried adding minimal changes to keep up the default ruff configuration (see https://github.com/astral-sh/ruff?tab=readme-ov-file#configuration). Please review carefully 🤓 !

Type of change

  • Maintenance

📚 Documentation preview 📚: https://pymc--7091.org.readthedocs.build/en/7091/

@juanitorduz juanitorduz marked this pull request as draft January 8, 2024 14:15
Copy link

codecov bot commented Jan 8, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (7bb2ccd) 92.21% compared to head (ad003e1) 92.23%.
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7091      +/-   ##
==========================================
+ Coverage   92.21%   92.23%   +0.02%     
==========================================
  Files         101      101              
  Lines       16912    16884      -28     
==========================================
- Hits        15595    15573      -22     
+ Misses       1317     1311       -6     
Files Coverage Δ
pymc/__init__.py 100.00% <ø> (ø)
pymc/backends/arviz.py 96.98% <100.00%> (+0.46%) ⬆️
pymc/backends/base.py 88.44% <ø> (+2.60%) ⬆️
pymc/distributions/bound.py 100.00% <100.00%> (ø)
pymc/distributions/continuous.py 97.53% <100.00%> (-0.27%) ⬇️
pymc/distributions/discrete.py 99.11% <ø> (ø)
pymc/distributions/multivariate.py 94.11% <100.00%> (+0.57%) ⬆️
pymc/distributions/transforms.py 98.48% <ø> (ø)
pymc/gp/hsgp_approx.py 95.59% <100.00%> (+0.59%) ⬆️
pymc/gp/util.py 96.47% <100.00%> (ø)
... and 24 more

... and 28 files with indirect coverage changes

@ricardoV94 ricardoV94 added maintenance no releasenotes Skipped in automatic release notes generation labels Jan 8, 2024
@juanitorduz juanitorduz marked this pull request as ready for review January 8, 2024 15:28
@juanitorduz juanitorduz requested a review from ricardoV94 January 8, 2024 15:28
pymc/backends/arviz.py Outdated Show resolved Hide resolved
pymc/model/core.py Outdated Show resolved Hide resolved
Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>
@juanitorduz juanitorduz requested a review from ricardoV94 January 8, 2024 17:33
@juanitorduz juanitorduz marked this pull request as draft January 8, 2024 20:32
@juanitorduz
Copy link
Contributor Author

@ricardoV94 Here are some additional improvements after your first review:

  • Revert line-length changes ✅
  • Remove all pylint mentions and comments (same for isort) ✅
  • Stricter rules trying to avoid global ignore (we just have one now). "Solved" many via #noqa
  • Moved some parts of setup.cfg into pyproject.toml. I wanted to remove it all but I need to take care of versioneeer (see 3130413). I will tackle this in another PR.

The pre-commit job goes from 55s -> 23s (on average) after the ruff change. Also, the project dependencies decreased :)

@juanitorduz juanitorduz self-assigned this Jan 8, 2024
@juanitorduz juanitorduz marked this pull request as ready for review January 8, 2024 21:10
@juanitorduz juanitorduz requested a review from ricardoV94 January 9, 2024 19:26
@juanitorduz
Copy link
Contributor Author

juanitorduz commented Jan 9, 2024

@ricardoV94 I had forgotten to include isort in the config! Now is included! 🙌

@ricardoV94 ricardoV94 requested a review from maresb January 11, 2024 10:53
Copy link
Contributor

@maresb maresb left a comment

Choose a reason for hiding this comment

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

This is pretty exciting. We should be able to get rid of pyupgrade too.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved

[tool.black]
[tool.ruff]
line-length = 100

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be "py39", but I don't think we want to tackle that in this PR.

Suggested change
target-version = "py37"

juanitorduz and others added 2 commits January 11, 2024 12:36
Co-authored-by: Ben Mares <services-git-throwaway1@tensorial.com>
Co-authored-by: Ben Mares <services-git-throwaway1@tensorial.com>
@maresb
Copy link
Contributor

maresb commented Jan 11, 2024

Similarly for pydocstyle with select "D".

@maresb
Copy link
Contributor

maresb commented Jan 11, 2024

The default rules in ruff are extremely minimalistic. I'd try selecting all of E and also W and check if you can make things stricter (possibly with exceptions) without blowing everything up.

It can be disappointing to have a previously-passing check that doesn't run, and then by the time you discover and fix it, it requires changing substantial parts of the codebase. (I've seen that happen with a malfunctioning line-length check, and then it's a battle to get that under control again.)

@juanitorduz
Copy link
Contributor Author

The default rules in ruff are extremely minimalistic. I'd try selecting all of E and also W and check if you can make things stricter (possibly with exceptions) without blowing everything up.

It can be disappointing to have a previously-passing check that doesn't run, and then by the time you discover and fix it, it requires changing substantial parts of the codebase. (I've seen that happen with a malfunctioning line-length check, and then it's a battle to get that under control again.)

When I add E I get 252 errors of because of long single docstrings" (E501). Can we tackle this in another PR? Otherwise, this initial change can take ages 😅 .

@juanitorduz juanitorduz requested a review from maresb January 11, 2024 12:37
@maresb
Copy link
Contributor

maresb commented Jan 11, 2024

When I add E I get 252 errors of because of long single docstrings" (E501). Can we tackle this in another PR? Otherwise, this initial change can take ages 😅 .

Of course, just add an ignore for E501.

The point is that there's lots of other things you'll pick up for free or almost for free.

@juanitorduz
Copy link
Contributor Author

@maresb Thanks for the feedback! I think addressed all your comments. Please let me know if I missed something 🙏

Copy link
Contributor

@maresb maresb left a comment

Choose a reason for hiding this comment

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

Looks excellent, great work!!!

@ricardoV94 ricardoV94 merged commit 2051d0b into pymc-devs:main Jan 11, 2024
22 checks passed
@ricardoV94
Copy link
Member

Thanks a ton guys!

@juanitorduz juanitorduz deleted the ruff_linter branch January 11, 2024 14:13
@juanitorduz
Copy link
Contributor Author

Thanks for your help! Please report any issue and I will try to solve it 💪

@ricardoV94
Copy link
Member

@juanitorduz I hear you are doing PyTensor next :D?

charliermarsh pushed a commit to astral-sh/ruff that referenced this pull request Jan 11, 2024
@juanitorduz
Copy link
Contributor Author

There is a PR open so I would not want to interfere 😅. I'll he happy to support though !

@ricardoV94
Copy link
Member

I think that one is stale, should be fine to take over (you can ask)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance no releasenotes Skipped in automatic release notes generation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ruff linter?
3 participants