-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1919540 - Implement basic environment handling for the search engine selector. #6474
base: main
Are you sure you want to change the base?
Conversation
Asking @mandysGit for the main review please, and @linabutler to take a look at the rust usage for us please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Rust parts look terrific; thanks, @Standard8!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I learned so much Rust reviewing this patch.
I left some suggestions and comments around code clarity, and few questions I had about things I didn't quite understand. No major blocking comments though.
/// Describes variations of this search engine that may occur depending on | ||
/// the user's environment. The last variant that matches the user's | ||
/// environment will be applied to the engine, subvariants may also be applied. | ||
pub variants: Vec<JSONEngineVariant>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding sub_variants underneath the pub variants
?
We could add this description:
subvariant description from search-config-v2 schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My plan was to add that in a later bug - this one is for handling of filtering for most of the environment.
@@ -127,6 +134,11 @@ mod tests { | |||
}, | |||
"extraField2": "123" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed there are these extraField
properties. What is it and and why do we need these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are testing that if add extra properties to the configuration in future then these will not cause existing clients to fail. The test description mentions these should be allowed, but I'll improve the assertion as well.
9701763
to
bc14cf9
Compare
Thank you both for the reviews, I've updated for the comments and believe I've addressed everything @mandysGit |
This adds most of the environment handling necessary for the search engine selector. There are some remaining parts that will be handled in bug 1930969.
This code is not currently used anywhere so no API / changelog actions are necessary.
Pull Request checklist
[ci full]
to the PR title.Branch builds: add
[firefox-android: branch-name]
to the PR title.