Proposal for new auto discovery results #427
Conversation
| pub struct NewUrlDiscoveryAttemptResult { | ||
| pub is_network_error: bool, | ||
| pub is_successful: bool, | ||
| pub is_the_site_url_same_as_the_user_input: bool, |
There was a problem hiding this comment.
| pub is_the_site_url_same_as_the_user_input: bool, | |
| pub is_the_site_url_same_as_the_user_input: bool, | |
| // Is this site running WordPress? | |
| pub is_wordpress_site: bool, | |
| pub struct NewUrlDiscoveryResult { | ||
| pub user_input_attempt: NewUrlDiscoveryAttemptResult, | ||
| pub other_successful_attempts: Vec<NewUrlDiscoveryAttemptResult>, | ||
| pub other_failed_attempts: Vec<NewUrlDiscoveryAttemptResult>, | ||
| } |
There was a problem hiding this comment.
I think Vec<NewUrlDiscoveryAttemptResult> might be part of the usability trouble.
WDYT about having something like:
| pub struct NewUrlDiscoveryResult { | |
| pub user_input_attempt: NewUrlDiscoveryAttemptResult, | |
| pub other_successful_attempts: Vec<NewUrlDiscoveryAttemptResult>, | |
| pub other_failed_attempts: Vec<NewUrlDiscoveryAttemptResult>, | |
| } | |
| pub struct NewUrlDiscoveryResult { | |
| // If the user provides a fully-formed valid URL, prefer that over any | |
| // "smart" things we might try to do | |
| pub user_input_attempt: NewUrlDiscoveryAttemptResult, | |
| // If the user didn't provide a scheme, we'll try HTTP and HTTPs | |
| // If they provide HTTP we'll try HTTPs just in case | |
| pub auto_https_attempt: Option<NewUrlDiscoveryAttemptResult>, | |
| // If the user didn't provide a scheme, we'll try HTTP and HTTPs | |
| pub no_https_attempt: Option<NewUrlDiscoveryAttemptResult>, | |
| // If the autodiscovery process results in a redirect | |
| pub redirected_attempt: Option<NewUrlDiscoveryAttemptResult>, | |
| pub is_successful: bool, | |
| pub is_failure: bool, | |
| // This is a copy of whichever of the above attempts was successful | |
| pub success: NewUrlDiscoveryAttemptResult, | |
| // Return the most detailed error we can – probably just the one | |
| // from the attempt that got the farthest in the process? | |
| pub error: UrlDiscoveryAttemptError | |
| } |
IMHO it's rare that you'd want to iterate through the attempts (failed or otherwise) without explaining to the user what you tried, so having explicit names for what we attempted would make the API more discoverable?
There was a problem hiding this comment.
We currently don't auto attempt http as it wouldn't be secure. If the user intentionally inputs an http url, then the discovery will work.
We probably wouldn't want both is_successful & is_failure since they are opposite of each other 😅
As I mentioned on Slack, I don't think the error belongs here as we can't make a good guess for which error should be raised. We need the error type to be in individual attempt result instead.
There was a problem hiding this comment.
Oh also, I don't think we are doing a separate attempt for redirects - but they might already work because the request executor handles it for us. I'd need a test case to double check this.
|
|
||
| #[derive(Debug, uniffi::Record)] | ||
| pub struct NewUrlDiscoveryResult { | ||
| pub user_input_attempt: NewUrlDiscoveryAttemptResult, |
There was a problem hiding this comment.
Curious if this can (or should) be a Result<NewUrlDiscoveryAttemptResult, UrlDiscoveryAttemptError> – that'd avoid the is_successful property (consuming code needs to branch either on a switch statement or on if is_successful so I don't think increases the costs)
There was a problem hiding this comment.
uniffi doesn't let us use Results in fields. They can only be used as return types for functions. The likely reason being, the errors get mapped to exceptions in other languages - as they don't all have a Result type that's similar to Rust.
No description provided.