HERA-Team / hera_cal

Library for HERA data reduction, including redundant calibration, absolute calibration, and LST-binning.
MIT License
13 stars 8 forks source link

Add ruff formatting #936

Closed steven-murray closed 6 months ago

steven-murray commented 6 months ago

Here I simply ran ruff fix . on the codebase (ruff is a linter and formatter that is super-fast and rolls flake8 and black and some other tools into one nice package).

@jsdillon I know you have some aversion to some of the black styling, but I thought it would be useful to run once on everything and give you a chance to have a quick look through to see if there's anything super objectionable here. If there is, we can always either:

  1. Switch off auto-formatting for that bit of code and keep automatic ruff formatting into the future.
  2. Keep the changes that ruff made here, but manually change those bits back, and not keep ruff as an automatic fixer in the future (so use this as a once-off style upgrade).
  3. There might also be a way to switch off some fixes in ruff so it does more what you want.

BTW I'm personally fine with essentially everything that ruff/black does automatically, except when you have a list of literals that has some structure (like a matrix), which I'd prefer to have visually keep that structure, but sometimes black rolls it out to one value per line. Not sure if such a thing exists here.

Note that many of the changes made here are unused imports etc. which can be problems.

Also tagging in @mkolopanis who might be interested, and knows a bit more specifically about ruff.

mkolopanis commented 6 months ago

regarding the structure thing you can usually do something like # fmt: off and later a # fmt: on to toggle auto-formatting. or skip for one liners you can see more here https://docs.astral.sh/ruff/formatter/#format-suppression

mkolopanis commented 6 months ago

also you can port all your flake8 configurations over to a pyproject.toml for ruff too https://docs.astral.sh/ruff/configuration/#configuring-ruff

jsdillon commented 6 months ago

I'm fine adding ruff, but I'd really like to not modify the things currently ignored by https://github.com/HERA-Team/hera_cal/blob/main/.flake8

My biggest issue is with how hard to works to keep individual lines short. You end up with stuff like this: image I personally find the new version substantially harder to read.

jsdillon commented 6 months ago

While I agree in general that all the odicts should be replaced with regular dicts, that's a separate PR. If you want to do that separately, I'll review it. Likewise, if you want to replace prints with actual warnings, that's yet another PR that I'd be in favor of as a separate thing. I'm really strongly against giant linting PRs that also have substantive code changes. They are terrible to review.

I really don't want to get into a discussion of whether the code is more or less readable in each particular instance. Some are improvements, some are steps backward. There's 33k lines of changes here... it's just not a good use of our time. I picked one instance from abscal because it comes at the front of the alphabet, not because I'm focusing on that particular module.

I'm fine running a linter to make a minimal set of changes so that flake8 (as defined in https://github.com/HERA-Team/hera_cal/blob/main/.flake8) passes. I'd really prefer not to go further than that at this time.

steven-murray commented 6 months ago

@jsdillon I understand the hesitancy. On the other hand it takes me time every time I write a new PR to manually fix the formatting, and furthermore the style in each module is inconsistent, which makes it hard to read. I can run the fixer with minimal settings as you suggest (and not run the formatter), but I think it's shooting ourselves in the foot long term. If instead we just bite the bullet and do one big PR that sets the style forever, we never have to worry about it again. But I'm not going to make that call. I'll rewind this PR and when I can I'll do another one with more minimal fixes.

jsdillon commented 6 months ago

I get that frustration. Can we integrate ruff (or similar) to always perform the minimal fix to new code so that it passes the style checks? That doesn't fix the issue of style being inconsistent between modules (which doesn't bother me and will still be the case in terms of function choices and variable names that reflect the original authors), but it does mean you won't have to manually fix any formatting, right?

steven-murray commented 6 months ago

Yeah, I will try that. Perhaps I'll try it once the few fresh PRs are merged