Conversation
tiftran
force-pushed
the
wcs
branch
2 times, most recently
from
0c1055a to
ccb9dcc
Compare
| /// Fetch World Cup data | ||
| WorldCup { | ||
| /// OHTTP relay URL | ||
| #[arg(long, default_value = "https://ohttp-merino.mozilla.fastly-edge.com")] |
There was a problem hiding this comment.
Nan said no need for OHTTP since no search data is being passed.
tiftran
force-pushed
the
wcs
branch
2 times, most recently
from
1c41220 to
fa10294
Compare
gruberb
left a comment
gruberb
left a comment
There was a problem hiding this comment.
I would recommend these changes. Nothing functional, but would be cleaner. Knowing that we likely remove all of that again after the event.
| .base_host | ||
| .unwrap_or_else(|| DEFAULT_BASE_HOST.to_string()); | ||
|
|
||
| let base_url = Url::parse(&format!("{}/api/v1/wcs/", base_host))?; |
There was a problem hiding this comment.
We could add the base url as a const (in addition to the DEFAULT_BASE_HOST? Or just do DEFAULT_BASE_URL?
And then we can use this in the tests too (import it there) instead of having another hardcoded url.
| }"#; | ||
|
|
||
| const LIVE_RESPONSE: &str = r#"{ | ||
| "current": [ |
There was a problem hiding this comment.
Another little thing is to use a const ICON_URL_BASE: &'static str = "https://storage.googleapis.com/merino-images-prod/logos/" maybe for the logos?
There was a problem hiding this comment.
actually i guess if we're storing those responses in json files, we can't do this right?
| pub teams: Option<String>, | ||
| } | ||
|
|
||
| pub trait HttpClientTrait { |
There was a problem hiding this comment.
They are all the same. Can we collapse it into one make_request?
pub trait HttpClientTrait {
fn make_request(&self, url: Url, params: WorldCupQueryParams) -> Result<Option<Response>>;
}| } | ||
| } | ||
|
|
||
| pub fn get_teams( |
There was a problem hiding this comment.
Could all just be make_request and then self.http_client.make_request?
There was a problem hiding this comment.
I mean, if we want to hardcode the endpoint to prevent typos etc, we have to use a fixed url here for each of the requests.
There was a problem hiding this comment.
i kinda do want to leave it, so that the endpoint is hardcoded here. Yes this leaves to some duplication, but maybe okay since it won't be sticking around for long?
| /// Creates a new `WorldCupClient` from the given configuration. | ||
| #[uniffi::constructor] | ||
| #[handle_error(Error)] | ||
| pub fn new(base_host: Option<String>) -> ApiResult<Self> { |
There was a problem hiding this comment.
We should pass a WorldCupConfig here instead of a String. And I don't think it can be optional.
There was a problem hiding this comment.
Suggest and CuratedRecommendations both have Option<String> and it defaults to the merino prod if it's not present, should I follow convention?
| WorldCupClientBuilder::new().build().unwrap().base_url | ||
| } | ||
|
|
||
| struct FakeHttpClientSuccess; |
There was a problem hiding this comment.
FakeHttpClientSuccess, FakeHttpClientNoContent, FakeHttpClientServerError differ only in their return values. With the trait collapsed to just one method (from the comment earlier), they reduce to one parameterized fake:
struct FakeHttpClient(fn() -> Result<Option<Response>>);
impl http::HttpClientTrait for FakeHttpClient { ... }
| pub use error::{ApiResult, Error, MerinoWorldCupApiError, Result}; | ||
| pub use schema::{WorldCupConfig, WorldCupOptions}; | ||
|
|
||
| pub(crate) const DEFAULT_BASE_URL: &str = |
There was a problem hiding this comment.
not sure if this is the best way to make it accessible for test.rs?
|
Added the accept language header and a "language" param, will remove/edit once there's a decision on how we're passing locale |
3 participants
This PR adds a new module to the merino crate that calls the Merino /api/v1/wcs endpoint
Also added a suggest subcommand to merino-cli for local testing.
Pull Request checklist
[ci full]to the PR title.