-
Notifications
You must be signed in to change notification settings - Fork 2
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
Added a clone method to request builders to implement copyWith #9
Conversation
lib/src/base_request_builder.dart
Outdated
|
||
/// Clones the current request builder using [clone] and applies the provided | ||
/// parameters. | ||
T copyWith({ |
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 value of the WithUrl method is to provide the ability to use the fluent API with arbitrary URLs returned by the service.
While this implementation is more flexible, it diverges from the other languages, and strays a bit in the general intent.
I'd encourage to align with the other languages for now and if we want to make a general improvement like this one, we'll coordinate it across languages
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.
Okay, so what's the best course of action now? Close this PR and maybe add something like withUrl
later?
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.
just renaming the method here, changing the parameters would be perfect I think.
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.
Ok, done
lib/src/base_request_builder.dart
Outdated
|
||
/// Clones the current request builder using [clone] and sets the given | ||
/// [template] as the url template. | ||
T withUrl(String template) => this.clone()..urlTemplate = template; |
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.
This method should also set the parameters with a single entry: key is going to be the raw url key you have in request information, value is going to be the value from the parameter
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'm afraid I don't follow... Can you point me to some other implementations of this method?
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.
@andrueastman can you chime in here?
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 believe what @baywet is saying is that the the withUrl
should not be setting the template but should be set with an an actual and valid Url(the request will be sent as is with the Url without having to template matching).
This is done by adding a value with a special key so that if its present, no template matching is done and the request is made directly. This is done to override the behavior of template matching in the event extra parameters are needed or a custom url is used.
https://github.com/microsoft/kiota-abstractions-dotnet/blob/4bff08d4ff90b24c705f71ba4d3baf89a85f31c0/src/RequestInformation.cs#L82C52-L82C61
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 moved the withUrl
method to an extension method since it only accesses public members. It in turn uses clone
, so in the end the class extending BaseRequestBuilder
only needs to implement a deep clone of all members.
For an example, take a look at the tests and how the SampleRequestBuilder
clones the path parameters using the spread operator.
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.
Thanks @ricardoboss
6e625e7
to
9f24f04
Compare
Fixes #2.
A
copyWith
method is a common pattern in Dart (or Flutter at least), so I added it as a convenience method to theBaseRequestBuilder
interface.LMK what you think.