Skip to content
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

Merged
merged 6 commits into from
Feb 15, 2024

Conversation

ricardoboss
Copy link
Member

Fixes #2.

A copyWith method is a common pattern in Dart (or Flutter at least), so I added it as a convenience method to the BaseRequestBuilder interface.

LMK what you think.


/// Clones the current request builder using [clone] and applies the provided
/// parameters.
T copyWith({
Copy link
Collaborator

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

Copy link
Member Author

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?

Copy link
Collaborator

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done


/// Clones the current request builder using [clone] and sets the given
/// [template] as the url template.
T withUrl(String template) => this.clone()..urlTemplate = template;
Copy link
Collaborator

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

Copy link
Member Author

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?

Copy link
Member Author

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?

Copy link
Collaborator

@andrueastman andrueastman Feb 14, 2024

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

Copy link
Member Author

@ricardoboss ricardoboss Feb 14, 2024

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ricardoboss

@ricardoboss ricardoboss merged commit 9c459ed into main Feb 15, 2024
1 check passed
@ricardoboss ricardoboss deleted the issues/2-with-url branch February 15, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consider adding a with url method in the base request builder
3 participants