Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Stop clearing unrecognized query params #6784
Stop clearing unrecognized query params #6784
Changes from 6 commits
f762d50
0313b4b
43037d5
013f8c8
d38e759
8d70f91
e26022e
8587d81
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It would be nice (but not necessary) to also have this handled in the internal CorpExperimentListDeepLinkProvider (the CorpDashboardDeepLinkProvider, on the other hand, would get this for free already with the way you've coded this).
Not necessary to do but if you don't do it, I want to ensure it's a conscious choice rather than an oversight.
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 did remember
CorpDashboardDeepLinkProvider
and . I did not rememberCorpExperimentListDeepLinkProvider
but it should be simple enough to update once this makes it's way to google3There 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.
Does it make sense for this to live in 'serializes feature flags'? Should it be its own test?
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 think it makes sense here because these are (were?) query parameters and they are being serialized.
If you feel strongly about it I'm happy to make another test.
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 don't understand what the additions to this test have to do with 'feature flags'. They don't seem necessary.
The comment for the test:
The name for the test:
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 fully read "feature flags" as "query params". I guess working on TB has created an alias for me