cf-units icon indicating copy to clipboard operation
cf-units copied to clipboard

Ruff: smaller steps

Open rcomer opened this issue 2 years ago • 5 comments

šŸš€ Pull Request

Description

SciTools/iris#353 proposed introducing ruff to cf-units for linting, but also changed some style choices (max line length and import sort order). IMO this made it difficult to spot what effect ruff was having on the code. This PR breaks it down so hopefully it's easier to see what's going on:

  • First commit takes the config file changes from SciTools/iris#353
  • Second commit reinstates the previous style choices in the config
  • Third commit shows ruff's autofixes. The majority of these modernise the string formatting, but there are also
    • removals of things that became redundant when we dropped python2
    • change dictionary creation from dict(...) to {...} - maybe for speed?
    • change if thing == None to if thing is None - I'm surprised flake8 didn't catch that one

CI will fail with remaining problems that ruff found:

  • Some of our comment lines are longer than 79 characters
  • In the tests we touch an object's attribute but don't get hold of it and use it after, which seems to be correct in context but the linter can't tell that

rcomer avatar May 03 '23 10:05 rcomer

@rcomer Thanks for this :rocket:

I was waiting for https://github.com/SciTools/iris/discussions/5254 to conclude, then reopen SciTools/iris#353, but totally happy to replace it with this :+1:

bjlittle avatar May 03 '23 11:05 bjlittle

Rebased, updated the ruff version and replaced black with ruff's formatter.

Swapping black for ruff's formatter only affects one line, shown in the most recent commit.

We now have more failures and fewer autofixes from main ruff. I think because some fixes that were previously done automatically are now marked "unsafe" so we would need to manually opt in (see https://astral.sh/blog/ruff-v0.1.0#respecting-fix-safety).

rcomer avatar Nov 13 '23 21:11 rcomer

I applied the "unsafe" fixes but then tweaked a few that didn't look quite right to me. Also tidied up the remaining errors so I think this is now ready for review.

Unlike most of my branches, the individual commits here may be meaningful enough to review individually.

rcomer avatar Nov 14 '23 17:11 rcomer

@SciTools/peloton would you be interested in taking this on, @tkknight ? We think you expressed interest in getting this one in before the Iris one, and that @bjlittle is unlikely to find time right now.

pp-mo avatar Nov 15 '23 10:11 pp-mo

New commit dismissed an approving review. Not seen that before šŸ˜•

rcomer avatar Feb 26 '24 16:02 rcomer

Hi @stephenworsley thanks for taking a look. I’m afraid I do not know if I will get to this this week. If anyone wants to push to my branch to get it through sooner, please do.

rcomer avatar Sep 24 '24 10:09 rcomer