MAINT: do fewer style checks

[bsipocz]

This turns off check for foo/2vs foo / 2, but still keeps checking for spaces around operators for better legibility, e.g. foo < bar and not foo<bar.

Similarly, we keep foo = 2 while bar(foo=2).

#470 (comment)

@msdemlei - if you have other specific error code you would like to turn off, please comment them here. The whitespace in arithmetic is a typically problematic one, so I fully agree that we can turn that off, but I find the rest of the examples above useful choices.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.38%. Comparing base (4d31a06) to head (ded4360).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #531   +/-   ##
=======================================
  Coverage   80.38%   80.38%           
=======================================
  Files          52       52           
  Lines        6189     6189           
=======================================
  Hits         4975     4975           
  Misses       1214     1214           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[msdemlei]

As people who've seen me in real life will know I'm probably not the most likely candidate to comment on style issues :-). But I am keen on switching off E226, so thanks for that.

I've looked at the output of a plain flake8 pyvo --count on a current checkout of pyvo, and here's my opinions (i.e., feel free to ignore them) on the diagnostics:

• E251 – I agree; I frankly can see scenarios when I might want to see whitespace for keyword defaults, but what I've spot-checked is not among them
• E124, E127 – I'm not a fan of visual indentation at all, so it's no surprise I'm not wild about these. I'd give them a 6 on a pain level range of 0 to 10.
• E123 – ok... wouldn't be my choice, but it doesn't seem unreasonable. Fine with me.
• E122 – ah. pyvo/discover/image.py:253 is indeed indented misleadingly. This seems like a useful check :-)
• E225 – the cases I spot-checked are ok, so I'm fine with keeping it on.
• E501 – I'd even make that one stricter :-) I will admit that I think that in cases like pyvo/discover/tests/test_imagediscovery.py:130, where there's a string one might want to update from what the machine generates, I sometimes like to break my own holy rules, but that's really minor convenience.
• E741 – that is: a variable name of l is off limits unconditionally? I mean, I won't publicly speak out in favour of variable names like "l", but forbidding them... I don't know. Still: pain level 0/10.

Thanks!

[bsipocz]

E501 – I'd even make that one stricter :-) I will admit that I think that in cases like pyvo/discover/tests/test_imagediscovery.py:130, where there's a string one might want to update from what the machine generates, I sometimes like to break my own holy rules, but that's really minor convenience.
E741 – that is: a variable name of l is off limits unconditionally? I mean, I won't publicly speak out in favour of variable names like "l", but forbidding them... I don't know. Still: pain level 0/10.

I have advocated for allowing longer lines as it helps with removing weird-looking and logically unnecessary line-breaks all the while most laptops and desktops come with a wide enough monitor to still allow two windows next to each other. So, I would like to keep the 110 characters as necessary.

As for single letter variables, the rule I believe comes from the ambiguity between l and I and more importantly (to me) to help with a quicker/easier search/replace. So, I would like to keep the rule as it may also help to prevent some bugs (anecdote only, but I recall we might have caught a few when this rule was turned on for the astropy core repo, but I may misremember)

I like the visual indent rules, but given their higher pain level and that it came up multiple times I would be OK to add them to the exception list.

[bsipocz]

@tomdonaldson @andamian - does this sound good to you?

[lmichel]

Having got used to working with a much stricter Pylint on another project, I ended up considering that the cost/benefit ratio of these rules was very positive. There are two rules which, if ignored, could affect the clarity of the code in my opinion:

• E124 (closing bracket does not match visual indentation) is sometime difficult to follow, especially when building dict. However, I think it provides a common rule to understand complex structures.
• E127: (Continuation line over-indented) personaly I appreciate the vertical alignment of the function parameters

[andamian]

I personally don't see a problem with E124 and E127. Sometimes E127 (Continuation line) gets on my nerves but it's for a good cause I think. Python is based on indentation, so why brake the flow with these exceptions?

[msdemlei]

On Tue, Mar 12, 2024 at 01:06:03PM -0700, Adrian wrote: I personally don't see a problem with E124 and E127. Sometimes E127 (Continuation line) gets on my nerves but it's for a good cause I think. Python is based on indentation, so why brake the flow with these exceptions?

So, I have not *exactly* researched them, and I'm holiday right now and hence can't be bothered at this point either, but from their messages it seemed they assume visual indentation, i.e., or so I understand the term, lining up along opening parentheses as in foo = long_function_name( arg1, arg2, arg3) I personally consider thta ugly, it intersubjectively is a pain without tool support, and, which is my major gripe, it wastes a ton of whitespace. True, you shouldn't nest too deeply anyway, but given I'm religious about 80 characters/line max, each step towards this indentation style is a pain for me. As I said: 6/10, which means I can live with it, but I'd much rather not.

[andamian]

I can't remember all the cases either. The ones in https://www.flake8rules.com/rules/E127.html and in the link provided make sense.

[bsipocz]

I also agree that E127 indentations are usually quite sensible, but also see the frustration for some of the recent cases when it's long string lines, etc.
So, if it was me, I would keep all these checks, but I'm happy to lift them if others find it helpful. (and as I said, my emacs flares up as a Christmas tree when I don't follow any of these rules, so an editor plugin can go a long way for ensuring the CI will pass on first try)

[msdemlei]

So, I've been trying to make PR#470 flake8-clean with

…

--ignore E252,E128,E251,W503,E226,E225,E13 which worked out; let me still say a few words why I'd still like to switch off E124 and E127. My main peeve with E124 is that it complains about this: return base64.b64encode(hashlib.md5(data).digest(), b"+%" ).decode("ascii")[:8]

-- but I *highly* prefer this to return base64.b64encode(hashlib.md5(data).digest(), b"+%".\ decode("ascii")[:8] or, even worse in my eyes, return base64.b64encode(hashlib.md5(data).digest(), b"+%" ).decode("ascii")[:8] In this case, it groks return base64.b64encode( hashlib.md5(data).digest(), b"+%").decode("ascii")[:8] which is ok for me, but still, I'd consider it less readable than the original. In most other cases, I went for backslash continuation. That's not a progress in my book, but as I said I can marginally live with it. And E127 forces me to say assert (cons.get_search_condition(FAKE_GAVO) == "(1.32521403e-24 BETWEEN spectral_start AND spectral_end)" " OR NOT EXISTS(SELECT 1 FROM rr.stc_spectral AS inner_s" " WHERE inner_s.ivoid=rr.resource.ivoid)") instead of the IMHO noticeably more readable assert (cons.get_search_condition(FAKE_GAVO) == "(1.32521403e-24 BETWEEN spectral_start AND spectral_end)" " OR NOT EXISTS(SELECT 1 FROM rr.stc_spectral AS inner_s" " WHERE inner_s.ivoid=rr.resource.ivoid)") So... for all I see, these style checks are mainly trading individual readability for avoiding whitespace adjustments in the change history (is that about right?). If that's the point, I'd frankly weigh readability higher in the cases of E127 and E124. Again, I'm not vetoing them, though, if someone sees great value in them.

[bsipocz]

I would say, let's go ahead with this PR to make life easier, and if it's too forgiving, we can always add back enforcing the line indents. (And of course, the skipped rules can still be followed even if we don't enforce them)

[lmichel]

The linter rules are easily reversible, so I see no reason to discuss more this PR.
I'd just like to give my point of view, which is that the blocking style rules must be chosen in such a way as to make the review work easier.
It's better to annoy coders a bit more with strict rules if it makes the reviewers' job easier.
This hardening would be acceptable if the GITHUB actions were modified so as to run falke8 on every push, whatever the branch. In this way, the MRs would only relate to clean code.