Replace ApiBaseUrl::new(&str) with a TryFrom<&str> implementation
#145
Conversation
wp_api/src/lib.rs
Outdated
| request_executor: Arc<dyn RequestExecutor>, | ||
| ) -> Result<Self, WpApiError> { | ||
| let api_base_url: Arc<ApiBaseUrl> = ApiBaseUrl::new(site_url.as_str()) | ||
| let api_base_url: Arc<ApiBaseUrl> = TryInto::<ApiBaseUrl>::try_into(site_url.as_str()) |
There was a problem hiding this comment.
| let api_base_url: Arc<ApiBaseUrl> = TryInto::<ApiBaseUrl>::try_into(site_url.as_str()) | |
| let api_base_url: Arc<ApiBaseUrl> = ApiBaseUrl::try_from(site_url.as_str()) |
This can be simplified as such ^, however I am not entirely sure if this is an improvement over the current API. The main issue I see is that ApiBaseUrl doesn't provide any way to construct itself besides the TryFrom which is not obvious without consulting the documentation. (Note that the ApiBaseUrl's fields are private, so it can't be constructed with struct literal syntax outside of its module which is why it's more important to think about how the consumers will construct it)
TryFrom is a good addition, but I suggest keeping the new function and calling TryFrom from it, just to cover the issue I mentioned above. If you agree with that, we can revert this line back to the original because it's a more natural way to construct the instance.
There was a problem hiding this comment.
Sounds good. The constructor is back in 5373cf1
wp_api/src/request/endpoint.rs
Outdated
| TryInto::<ApiBaseUrl>::try_into("https://example.com") | ||
| .unwrap() | ||
| .into() |
There was a problem hiding this comment.
| TryInto::<ApiBaseUrl>::try_into("https://example.com") | |
| .unwrap() | |
| .into() | |
| ApiBaseUrl::try_from("https://example.com").unwrap().into() |
2 participants
What says in the title.