search

ImagingDataCommons / highdicom

High-level DICOM abstractions for the Python programming language
https://highdicom.readthedocs.io
MIT License
171 stars 37 forks source link

Enforce ruff/pyupgrade rules (UP) #287

Closed DimitriPapadopoulos closed 3 months ago

DimitriPapadopoulos commented 4 months ago

This is a suggestion of Scientific Python Repo-Review.

CPBridge commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

I'll look into fixing line length next week.

@CPBridge I fixed the line length problems.