Activity worker refactor

[dandavison]

This is pure refactoring for readability. I found it very helpful while creating a Nexus worker that is similar in some respects to the activity worker. All this PR does is move us to this pattern:

        try:
            await self._execute_activity(start, running_activity, completion) # 170 lines
        except BaseException as err:
            # 80 lines of error handling plus sending completion

i.e. separating the execution logic from the error handling, since it's very hard to read a try...except with 170 lines in the middle.

[cretz]

Not the biggest fan of de-inlining single-use logic for running an activity from run_activity when it is only used in one place. If anything, I could understand exception handling being de-inlined if it was really too long for you. But not enough disagreement to prevent the change.

[dandavison]

Not the biggest fan of de-inlining single-use logic for running an activity from run_activity when it is only used in one place. If anything, I could understand exception handling being de-inlined if it was really too long for you.

As explained in the intro, the logic inside the try...except was 170 lines long. That's always considered unacceptably long in Python codebases in my experience, and it's standard to de-inline in such situations even though it will be single use. Standards may differ in some compiled languages, but in dynamic languages like Python there's a strong emphasis on readability because of the risk of making mistakes that won't be discovered until run-time (and that risk was even greater in the pre-static-typing era in which Python coding style conventions became establised).

[cretz]

As explained in the intro, the logic inside the try...except was 170 lines long. That's always considered unacceptably long in Python codebases in my experience

Meh mostly only long because of the forced wrapping, it's not that many statements. Regardless, if this is too long for you, my suggestion is to extract out the except body, not the try body. Forcing users to hop so many levels deep for the primary logic makes code hard to understand.

[dandavison]

Regardless, if this is too long for you, my suggestion is to extract out the except body, not the try body. Forcing users to hop so many levels deep for the primary logic makes code hard to understand.

As I've said, in Python codebases with good style conventions, one never places 170 lines nested inside a try statement. You might have missed the addition I made:

Standards may differ in some compiled languages, but in dynamic languages like Python there's a strong emphasis on readability because of the risk of making mistakes that won't be discovered until run-time (and that risk was even greater in the pre-static-typing era in which Python coding style conventions became establised).

[dandavison]

@cretz can you help resolve this TODO / question?

[cretz]

I think it's probably a mostly true statement because the _run_activity is written to hopefully trap exceptions, but I think the intention was to set return_exceptions=True instead of False below, so that's probably what we should do here.

[dandavison]

Thanks, yes that's exactly what I was thinking. Updated TODO comment to that effect and leaving it in.

[cretz]

I would be totally ok changing the parameter

[dandavison]

OK, yes let's investigate that. But this is a pure refactoring PR and I don't like mixing functional changes with refactoring.