Simplify uniffi api discovery result #652
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
crazytonyli
approved these changes
Apr 8, 2025
| // Sort in descending order so the most important error is returned | ||
| .sorted_by(|a, b| b.compare_importance(a)) | ||
| .next() | ||
| .expect("If the discovery was unsuccessful, there is at least one error")) |
Contributor
There was a problem hiding this comment.
Nitpick: I think you can get rid of this unnecessary expect if deleting find_successful and re-implementing it within this function.
Contributor
Author
There was a problem hiding this comment.
I considered that, but that function should still be useful for Rust clients. I also considered duplicating the implementation, but the expect didn't bother me enough to do that 🤷
This was referenced Apr 8, 2025
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since api discovery is done through multiple attempts, it can be a little difficult to work with its result especially when it results in an error. To help combat this, this PR introduces the concept of "error importance" for
AutoDiscoveryAttemptFailureso that we can identify which error would be the most useful to share with the user. This is currently closely related to when the error occurs in the api discovery process where the errors from later steps are treated as more important.This concept is applied as a helper in the Rust API using the new
combined_resultfunction. However, for native clients, it removes the previousAutoDiscoveryUniffiResultin favor of directly returning this combined result. This means the clients can no longer show the result of each attempt, but that wasn't being useful at the moment. If we ever want to make the result of all attempts available, it'll be a simple change.