Apply ruff flake8-simplify

[swrichards]

https://docs.astral.sh/ruff/rules/#flake8-simplify-sim

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 90.74074% with 5 lines in your changes missing coverage. Please review.

Project coverage is 94.22%. Comparing base (e9cf977) to head (090ed7c).

Files with missing lines	Patch %	Lines
src/open_inwoner/accounts/views/auth.py	50.00%	2 Missing ⚠️
.../open_inwoner/components/templatetags/form_tags.py	80.00%	1 Missing ⚠️
src/open_inwoner/openzaak/import_export.py	75.00%	1 Missing ⚠️
src/open_inwoner/utils/views.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1614      +/-   ##
===========================================
+ Coverage    94.20%   94.22%   +0.01%     
===========================================
  Files         1083     1083              
  Lines        39985    39955      -30     
===========================================
- Hits         37669    37646      -23     
+ Misses        2316     2309       -7     

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

[swrichards]

@pi-sigma Not much of a rush on this one, but here's a first shot at introducing some useful rules. Reviews on this kind of PR is essentially comparing if the changed lines are functionally equivalent. I've split this up into a commit with "safe" fixes and another one with "unsafe" sixes (these are the terms used by ruff when it can and cannot apply fixes relatively blindly). The unsafe ones were mostly trivial, but I did notice that in a few cases it required a bit more thought to get right.

If you feel there are cases of rules where the changes actually caused a regression in readability feel free to flag it, we can discuss and possibly revert with an ignore statement.

[pi-sigma]

Overall nice, left a few comments for discussion. General suggestion: it might be good to get some input from people working on other projects (Open Forms, Open Zaak etc.). Not that we have to be totally aligned, but it would be nice to go at least ruffly in the same direction when it comes to coding style (for example, I'm not a huge fan of booling everything everywhere, which comes up in two of my comments; but perhaps this is the one true good style and I better get used to it).

[pi-sigma]

This is a limiting case of SIM102 ("Use a single if statement instead of nested if statements"). The original is more readable IMHO than the new version. On the other hand, the rule is generally good. I'm leaning towards not enforcing this with an automatic linter but instead adopt the convention to follow the collapse rule - if and when it makes sense.

[pi-sigma]

Another case where I find the original more readable, though I would have preferred:

if request.user.is_staff and request.user.is_verified():
    return False
return True

which makes disabling the toolbar the default and is closer to how we would express the idea in English (bool(not FOO or not BAR) sounds rather esoteric in comparison).

[swrichards]

I think this is an interesting case. I agree the not.. not.. is not very readable, but in general I think having a False and True return flowing from a conditional is unpleasant to read. But some of this can simply be written to be more explicit, so something like:

    def force_disable_toolbar(self, request):
        is_non_staff_user = not request.user.is_staff
        is_unverified = not request.user.is_verified()
        return is_non_staff_user or is_unverified

My sense is that the application is mostly difficult to read when there's a lot of terms in the conditional, the auto-fix is not great and that can be a nudge to group the terms in more semantically meaningful terms.

[pi-sigma]

Interesting. I don't feel the same about returning True or False from a conditional. Perhaps because I'm reading a function/method that returns bool as a question: "disable_toolbar? if FOO, then yes/true, if BAR, then no/false". However, your proposal would also work for me (though I would use user_is_not_staff and user_is_not_verified to get user_is_not_staff or user_is_not_verified).

[pi-sigma]

This is similar to the previous case. Here too I find the original more readable.

[swrichards]

I think this is generally a good rule as my sense is that in more cases than not it will lead to more idiomatic Python (and more optimized: the builtins are often faster). But I agree here it becomes a bit unreadable so I disabled it.

[pi-sigma]

Interesting. I don't feel the same about returning True or False from a conditional. Perhaps because I'm reading a function/method that returns bool as a question: "disable_toolbar? if FOO, then yes/true, if BAR, then no/false". However, your proposal would also work for me (though I would use user_is_not_staff and user_is_not_verified to get user_is_not_staff or user_is_not_verified).

[pi-sigma]

Again I prefer the original. By way of contrast: if you store not request.user.is_authenticated in a variable user_is_not_authenticated, you preserve all information from the original. The two have the same meaning, and anyone who reads user_is_not_authenticated on its own knows what it means.

That's not the case here: with auth_required_regular_user you loose information. If I read the variable on its own I don't know what it means. In particular, the variable name doesn't convey that the user is not authenticated. In order to know this, I have to go back up in the code to check. Then what's the point of storing this condition in a variable to begin with? The ruff version doesn't do this, but it has the drawback (IMHO) of a convoluted conjunction/disjunction...

[swrichards]

That's not the case here: with auth_required_regular_user you loose information. If I read the variable on its own I don't know what it means.

Can you elaborate: do you mean that because it's in a separate variable, you don't immediately realize it's going to be used in the conditional, and therefore feels context-less? (To that I would argue that using it in the next statement should be sufficient context), but perhaps I misunderstood.

[pi-sigma]

"using it in the next statement should be sufficient context": yes, but the variable doesn't add any value because it's not used anywhere else. It's only purpose is to help appease the linter even though the original was just fine, and this shows (in my view). However, this is really a minor issue. We can go along with this and re-evaluate if cases like this come up again.

[swrichards]

Ah I think we've been talking at cross-purposes, at least about the area of disagreement.

The linter requirement was simply to not have two conditional branches executing the same code (which I think is a useful rule), my moving the conditions to variables is because I think it makes the conditional clearer than this kind of conditional:

                if (
                    ext.requires_auth and not request.user.is_authenticated
                ) or (
                    ext.requires_auth_bsn_or_kvk
                    and request.user.login_type
                    not in [
                        LoginTypeChoices.digid,
                        LoginTypeChoices.eherkenning,
                    ]
                ):
                    nodes.remove(node)
                    continue

But ruff is perfectly happy with this. But to be clear, I think moving the conditions into variables would be an improvement even in the original code, because I think it more clearly expresses the intent (otherwise you have to reason harder: why this juxtaposition of conditionals?).

In any case, that's orthogonal to the PR, I am happy to revert and just consolidate the original conditional with or.