Refactor unneeded 'continue' jumps in jobs

[eumiro]

No description provided.

[Taragolis]

My personal opinion: nothing bad with break in try block, try ... else do not add viable benefits here.

[eumiro]

try block should contain only code that is expected to throw an exception.

[potiuk]

I started to like more the try/except/else. It does have a nice property where it clerly separates the "exception handling" logic with "no exception" logic. In this case it does not add much value, but I think there is an educational value. I think one reason we do not use it is that it's somewhat under-used (even if pretty old) Python language construct and by using it in more places in Airflow we might get people learn more about it and use more of it.

[Taragolis]

We step on the field: tabs vs spaces and " vs '.

According to the documentation for python

The try … except statement has an optional else clause, which, when present, must follow all except clauses. 
It is useful for code that must be executed if the try clause does not raise an exception.

Personally I do not know the tool which automatically replace it, so I can't see how we can't prevent new code without else clause appear in codebase. UnboundLocalError should in theory prevented by ruff or mypy anyway

[potiuk]

I tihink we can't enforce it, but we can at least have a lot examples in our code that people learn to use it.

But yes. this is a bit of "tabs vs. spaces". Maybe worth discussing it at the devlist what we prefer if this is controversial and get to lazy consensus or even vote, This has been done multiple times in the past for various conventions we use. We even enforced some of those. And as everything here - as long as we agree on something as a convention, the "disagree but engage" rule will kick in and we will all follow.

[potiuk]

so @eumiro - maybe that's a good point to start a discussion on devlist where you would explain and ask people for opinions (just be prepared for a bit of flamewar)

[Taragolis]

First of all I'm not against rule "use else in try..except", we just need to use in places where it actually need, rather than always. IMO this is informal rule, in category "might/might not" and not in "should/should not" and definitely not in "must".

I'm sure that we have a rule readability first and in previous version it very easy to see what is going on:

1. Try to commit
2. If success exit from loop
3. If OperationError happen ...

In new version

1. Try to commit
2. If OperationError happen ...
3. (in previous series of try..except tv series 🤣 ) if no exception happen then exit from loop

Someone could also appeal to the fact that else in try...except would cost additional ... couple nanoseconds (OMG in Application which communicate with DB right after commit 🤣 ), and we could easily start to play code golf and optimise in place where it not required.

[Taragolis]

And we just started tabs vs spaces and " vs '

I've also have another topic for "Python Holy War": "use := since we have Python 3.8 or try to avoid this operator"

[potiuk]

:D

[dstandish]

I think we should avoid doing cosmetic refactors like this. Often times with a cosmetic refactor you risk screwing something up, and you make the git history more fragmented and potentially harder to revert things or cherry pick things. For example, I was hoping to revert the backfill fix for dask executor (f616ee8) because we don't need it anymore and it makes the code more complicated. But, the code was changed again in this PR so it would not be easy to revert.