Skip to content

Update pre-commit #368

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

Merged
merged 5 commits into from
Jul 28, 2024
Merged

Conversation

jessegrabowski
Copy link
Member

Closes #350

Switches our pre-commit to use ruff. I also changed the pyupgrade to version 3.10, since that's the minimum we support these days.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jessegrabowski jessegrabowski requested a review from maresb July 26, 2024 11:13
@maresb
Copy link
Collaborator

maresb commented Jul 26, 2024

Tests are failing with fixture 'rng' not found

@maresb
Copy link
Collaborator

maresb commented Jul 26, 2024

This would be easier to review with code fixes in a separate commit.

Ruff works much better if rules have been explicitly defined in pyproject.toml.

Adding the rule UP makes pyupgrade mostly redundant, although last time I checked pyupgrade did a more thorough job.

@jessegrabowski jessegrabowski force-pushed the pre-commit-update branch 2 times, most recently from b16789a to fd0e508 Compare July 26, 2024 12:34
@jessegrabowski
Copy link
Member Author

I rolled back everything except the actual changes to the pre-commit config. I just copied the pymc ruff configuration, except I removed the global exception for F841, except for tests.

If you are happy with where its at, I'll actually run the pre-commit.

I switched to using the ruff pyupgrade. The difference is that it forces you to make the changes yourself, whereas pyupgrade does it automatically. I'm lazy so I like pyupgrade, but maybe one less dependency is worth it.

Copy link
Collaborator

@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.

Nice! This looks great now!

@jessegrabowski
Copy link
Member Author

@theorashid can you have a look at this and make sure I didn't miss anything you wanted to see in the configuration?

Regarding the points raised in #366 :

  • I put #noqa on gp.latent_approx.py L144 mu = self.mean_func(X). Not sure if it is supposed to be there, but I kept it (@bwengals )
  • The rv_op thing in statespace didn't trigger, but I'll look into it.

For the root directory cleanup questions, you should open a separate issue (and I promise and I won't step on your toes again, sorry)

Copy link
Collaborator

@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.

The difference is that it forces you to make the changes yourself, whereas pyupgrade does it automatically. I'm lazy so I like pyupgrade, but maybe one less dependency is worth it.

Sorry, GitHub glitched and I didn't see your comment when I previously approved. Give this a whirl and it should fix (almost) everything automatically now.

If you really truly love your reviewer then you can separate your commits even further via git commit --no-verify to commit the automatic fixes while leaving in the remaining errors so that by-hand fixes can be viewed separately.

@jessegrabowski
Copy link
Member Author

I do really love my reviewer, so I did as you asked.

Copy link
Collaborator

@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.

Immaculate!

@theorashid
Copy link
Contributor

I'm happy with this. I'll close my PR and let's close the original issue too. Maybe I'll pick up more typos when we clean up the project root

@jessegrabowski jessegrabowski merged commit 89bbe19 into pymc-devs:main Jul 28, 2024
8 checks passed
@jessegrabowski jessegrabowski deleted the pre-commit-update branch February 8, 2025 05:52
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.

Update pre-commit hooks to use ruff and ruff-format
3 participants