Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Factor error-handling out of poll method #1187

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Dec 19, 2024

This is a follow-up to #1063 — it's very lightly revised from commits that were in my draft branch leading to that PR, and which I left out at the time because the PR was getting long and complex enough already.

In this PR, factoring this logic out of the [UpdateMachine.poll] method isn't for the sake of deduplicating anything: the methods the logic gets moved into are still private helpers tuned specifically for the needs of that method. But it allows that method to get quite a lot shorter, so its overall structure is more apparent. And it also allows each of the two phases of error-handling to be read in a bit more of a self-contained context, by having them in their own methods.

This method has gotten awfully long and tangly.  Here's a first step
on giving it some more organized structure.
This logic pairs closely with _maybeReportToUserTransientError,
so pulling it out lets that logic live nearer each other.
This is basically a form of error reporting.
Now the `poll` method itself is a lot more focused on the overall
structure of how polling works.
This takes it a bit farther in squeezing out details that aren't
part of the main picture of what this method is doing.

At this point the main remaining thing that feels like clutter
is all the `if (_disposed) return;` lines.  But I don't have any
ideas at the moment for a better way to handle those.
@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Dec 19, 2024
Copy link
Member

@PIG208 PIG208 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The result looks cleaner. I found the commit store [nfc]: Move polling backoff/reload inside error helpers to be slightly harder to read, but overall makes sense to me. Left a comment.

/// See also:
/// * [_handlePollRequestError], which handles certain errors
/// and causes them not to reach this method.
Future<void> _handlePollError(Object error) async {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this helper becomes the only user of _unexpectedErrorBackoffMachine, would it be better to move that BackoffMachine here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants