Skip to content

Conversation

jkmassel
Copy link
Contributor

Adds tests (and types) that were missing from #44, including:

  • A proper type to represent an OAuth URL response
  • A WPAPIApplicationPasswordDetailsBuilder for parsing the OAuth URL
  • Appropriate error types to handle failure to parse the OAuth URL response
  • Non-panicing WPRestAPIURL handling

@jkmassel jkmassel requested a review from oguzkocer March 21, 2024 03:24
}
}

impl TryFrom<WPRestAPIURL> for url::Url {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if we want to make this a recoverable error – providing an invalid URL is a programmer error, so this should probably panic instead? But a server could also provide an invalid URL and that's not the user's fault.

Interested to hear y'alls thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

That'd be a none issue if WPRestAPIURL is guaranteed to be a valid URL, right? We can un-pub WPRestAPIURL.string_value and use a WPRestAPIURL::from_str() -> Result<WPRestAPIURL, UrlParsingError> function as the only function to create a WPRestAPIURL instance?

};
}

builder.build() //.map_err(|err| UrlParsingError::InvalidUrl)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, the previous implementation using fold is cleaner. The build function body can be merged into this function here.

}
}

impl TryFrom<WPRestAPIURL> for url::Url {
Copy link
Contributor

Choose a reason for hiding this comment

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

That'd be a none issue if WPRestAPIURL is guaranteed to be a valid URL, right? We can un-pub WPRestAPIURL.string_value and use a WPRestAPIURL::from_str() -> Result<WPRestAPIURL, UrlParsingError> function as the only function to create a WPRestAPIURL instance?

extension OAuthResponseUrl {
static func new(stringValue: String) -> OAuthResponseUrl {
OAuthResponseUrl(stringValue: stringValue)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't find any caller to this function. Can it be deleted?

@jkmassel
Copy link
Contributor Author

jkmassel commented Jul 4, 2024

Replaced by #163 – closing

@jkmassel jkmassel closed this Jul 4, 2024
@jkmassel jkmassel deleted the add/url-tests branch November 20, 2024 05:29
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