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

src: lore_api_client: use color_eyre::Result #26

Open
wants to merge 2 commits into
base: unstable
Choose a base branch
from

Conversation

OJarrisonn
Copy link
Contributor

FailedFeedRequest now is a valid Report so can be used in Result type from color_eyre.

All other changes are collateral.

This is part of #2

`FailedFeedRequest` now is a valid `Report` so can be used in `Result`
type from color_eyre.

All other changes are collateral.
Copy link
Collaborator

@davidbtadokoro davidbtadokoro left a comment

Choose a reason for hiding this comment

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

Hi @OJarrisonn, and thank you for this great PR! I've left some comments due to stuff that is kind of my fault 😬

In any case, I think we may need to discuss quickly offline, as I was testing this and getting some weird results. Maybe is my misunderstanding about the way erye works, but I'll ping you later.

Thank you so much anyway, as this is a valuable change!

Comment on lines -79 to -83
match failed_feed_request {
FailedFeedRequest::UnknownError(error) => bail!("[FailedFeedRequest::UnknownError]\n*\tFailed to request feed\n*\t{error:#?}"),
FailedFeedRequest::StatusNotOk(feed_response) => bail!("[FailedFeedRequest::StatusNotOk]\n*\tRequest returned with non-OK status\n*\t{feed_response:#?}"),
FailedFeedRequest::EndOfFeed => (),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I forgot to mention in our offline conversation that it is important for the caller to handle the variants of FailedFeedRequest. Especially EndOfFeed, which is an edge case when we navigate through the whole history of a list. Below, is an explanation of why we need to match here, but my bad on my end for not explaining this earlier... Try accessing a small list (I found the ath12k to have ~1500 patchsets) and navigate to the end of the feed to see the panic happening (it may take a while without the VPN stuff we discussed).

Explanation:

Because of logic on the LoreSession type, we make subsequent fetches until we hit the number of patchsets we want, so if the page size is 30 and there are 45 patchsets total on a list when the user prompts for the second page, the program would panic, as it would try to access something like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So in this case, if the request returns an Err(EndOfFeed), this caller should return an Ok(()) rather than propagating the error

My fault, didn't read the code with enough attention

src/lore_api_client.rs Show resolved Hide resolved
src/lore_api_client.rs Show resolved Hide resolved
src/lore_api_client.rs Show resolved Hide resolved
src/lore_api_client.rs Show resolved Hide resolved
src/lore_api_client.rs Show resolved Hide resolved
src/lore_session.rs Show resolved Hide resolved
src/lore_session.rs Show resolved Hide resolved
Comment on lines -54 to +55
Err(failed_feed_request) => return Err(failed_feed_request),
Err(failed_feed_request) => bail!(failed_feed_request),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here about recoverable errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here about bailed errors still being recoverable

@davidbtadokoro
Copy link
Collaborator

Hi @OJarrisonn, and thanks for the quick update!

So in this case, if the request returns an Err(EndOfFeed), this caller should return an Ok(()) rather than propagating the error

My fault, didn't read the code with enough attention

Actually, for the caller we need to maintain the same previous handling of the Err variants, i.e., "handle" EndOfFeed by ignoring it, and throwing the other two variants.

@davidbtadokoro
Copy link
Collaborator

davidbtadokoro commented Aug 16, 2024

This may get a little complex:

The point of this PR is to use color_eyre's Result since we can cast any Report type (how color_eyre calls valid error types) into the Err variant.

This allows the .text().unwrap() at line 60 to become a .text()?, so instead of panicking we propagate the error when calling .text().

So this function may return 2 different kinds of errors: a FailedFeedRequest or a reqwest::error::Error. Those two errors are propagated as a Report, but the caller may use a downcast to recorver from this error

Yes! After I posted the review I went and digged further and found that bail! won't cause a panic if the upper callers handle it, so it is indeed the behavior we intend.

However, when I made allusion to "weird behavior" was because, even if we handle the EndOfFeed in fetch_current_page, by matching the downcasted Report type returned, the application doesn't panics as expected, but the UI kind of "melts". Try it for yourself to see if you can reproduce it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants