-
Notifications
You must be signed in to change notification settings - Fork 320
Add set_query_pair method to UrlExt trait #3331
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds a new set_query_pair method to the UrlExt trait to simplify setting query parameter key/value pairs with automatic overwriting of existing values.
Key changes:
- Added
UrlExt::set_query_pair()method that overwrites existing query parameters with the same key - Comprehensive test coverage including edge cases like duplicate keys, special characters, and method chaining
- Updated CHANGELOG to document the new feature
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| sdk/core/typespec_client_core/src/http/mod.rs | Added set_query_pair() method to UrlExt trait with implementation and comprehensive tests |
| sdk/core/typespec_client_core/CHANGELOG.md | Documented the new UrlExt::set_query_pair() method under Features Added |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
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.
Code seems fine if only calling once, but I suspect that isn't the case.
A convenience method for setting query parameter key/value pairs that will overwrite an existing key with the new value.
e68a8f4 to
9a1a816
Compare
heaths
left a comment
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 like the shape of the API, but you're not going to be able to use string slices into the Url once you start manipulating. You'll need to call into_owned() for each key and value like we were before, or clone the Url to use slices which might be cheaper - certainly less fragmented - but @LarryOsterman doesn't think that it's much of an issue on Windows and linux, at least, these days. So maybe we don't need to over-optimize now as long as we make sure the API could work either way.
But since when did order matter? The existing way we use in client methods doesn't preserve order. Params we keep end up first and params we replace/add end up coming last. This seems like a new and unnecessary requirement, and one I don't think other languages do. It shouldn't matter anyway except maybe for array params to combine them in order LTR.
A convenience method for setting query parameter key/value pairs that will overwrite an existing key with the new value.
Fixes #1907