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

Enforce ruff/pyupgrade rules (UP) #287

Merged
merged 5 commits into from
Jun 15, 2024

Conversation

DimitriPapadopoulos
Copy link
Contributor

This is a suggestion of Scientific Python Repo-Review.

UP008 Use `super()` instead of `super(__class__, self)`
@CPBridge
Copy link
Collaborator

Hi @DimitriPapadopoulos

Thanks for the PR. We currently use flake8 as a linter, which overall does a pretty good job. However these other tools appear to catch some other potentially useful things so I'm happy to make these changes.

However there is one issue in that our flake8 settings require <=80 character lines (@hackermd 's has some strong feelings about this...), and one of the tools (ruff?) appears to be reformatting lines to be longer than this limit, which is causing our current CI pipeline to fail. I'm not keen to change the line length limit as it has been in place since the start and we would have inconsistently formatted code everywhere. Is it possible to change the settings to prevent reformatting beyond 80 chars?

Also, would it make sense to add these tools to the CI so that we continue to remain in compliance?

@DimitriPapadopoulos
Copy link
Contributor Author

I agree flake8 does a good job, changing might not a priority. For what it's worth, the Scientific Python Development Guide mostly suggests:

  1. moving from setup.py/setup.cfg to pyproject.toml, which is probably the most important item,
  2. using ruff for linting and formatting, with a minimal set of rules including bugbear and pyupgrade,
  3. using pre-commit - I am not a fan of pre-commit but eventually got used to it.

I'm not sure these suggestions have anything to do with compliance. I expect they will remain mere suggestions in the foreseeable future. But then I must admit I have become used to code formatted with ruff, and find it's a good idea to follow the trend.

To answer your question, yes, we can set ruff to use 80 chars/line. The default for ruff is 88 and a default maximum of 88 to 100 chars/line seems more reasonable nowadays. But again, we can set it to 80.

You may use only the ruff linter. If you do use the ruff formatter, it will probably change some code any way - so that would probably be a good moment to switch to 88 chars/line or more.

While the bugbear rules do detect actual issues (like a missing assert in tests), the pyupgrade rules mostly renovate the code and may improve performance - although I suspect such "random" performance improvements are usually not perceptible at the scale of a whole Python module.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented May 29, 2024

I'll look into fixing line length next week.

@CPBridge I fixed the line length problems.

UP032 Use f-string instead of `format` call
UP034 Avoid extraneous parentheses
UP039 Unnecessary parentheses after class definition
UP015 Unnecessary open mode parameters
@DimitriPapadopoulos DimitriPapadopoulos changed the title Enforce ruff/pyupgrade rules Enforce ruff/pyupgrade rules (UP) Jun 2, 2024
@CPBridge CPBridge merged commit 6652a16 into ImagingDataCommons:master Jun 15, 2024
13 checks passed
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.

2 participants