[radoering]

Pull Request Check List

Relates-to: #4776

I plan to have one commit per violation code, so you can easily check if you like all of them.

Before I tackle SIM300 Yoda conditions (which is by far the most frequent violation): You also think that this should be fixed, don't you? (Just want to make sure because I know some people who like Yoda conditions). Hopefully, most of these conditions can be replaced with some sort of regex, I still have to come up with. 😀

[finswimmer]

Before I tackle SIM300 Yoda conditions (which is by far the most frequent violation): You also think that this should be fixed, don't you?

Yes, should be fixed :)

[neersighted]

The initial work looks good, and the list of lints at the attached repo also look very helpful for ensuring readable code. As long as the lint isn't too aggressive and the error messages are helpful (don't want to overwhelm new contributors), implementing this looks good to me.

[radoering]

I fixed all violations except for SIM106: Handle error-cases first because I think in many cases the code would be more difficult to understand resp. maintain after fixing SIM106 than before fixing it.

If the violation is in a simple if-else it might be ok. One could debate whether an additional not or an != instead of == in the condition is worth the exception being thrown first, but if the violation is in an if-elif-...-else chain, it becomes really ugly (in my opinion). Let's take the violation at src/poetry/puzzle/solver.py:298 for example:

if not previous:
    # do something
elif dep:
    # do another thing
else:
    raise ValueError

In order to handle the error first, I have to add:

if not (not previous or dep):
    raise ValueError

which could be simplified to

if previous and not dep:
    raise ValueError

But what happens if an additional case has to be added (another elif)? The condition of this case has to be merged into the error handling condition! Thus, you must not forget to do that and you have to be careful not to make a mistake doing it. This seems like some sort of complicated code duplication to me...

In my opinion, SIM106 should be added to the ignore list.

@finswimmer, @neersighted: What do you think?

[neersighted]

@neersighted: What do you think?

I do think that the general structure proposed by SIM106 is more readable, but I agree that long-term maintainability may be compromised if the pattern is not easily internalized.

I'm fine with disabling the check for now, and revisiting later, personally. All the other changes on this PR look like pretty unambiguous wins for readability and maintainability.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.