-
Notifications
You must be signed in to change notification settings - Fork 214
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
Do not override user's settings from config #3401
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.
Thanks for starting this. To reduce the amount of code we're projecting, I'm suggesting to add TryAdd methods in the headers classes in abstractions. Thoughts?
This change is going to be not retro compatible and would require, at minimum a
If we decide to go down this path I'd propose we go full-in and refactor the entire logic into abstractions, something like: generated: public async Task<double?> MethodName(string b, string c, Action<RequestConfig> config = default) {
#endif
if(string.IsNullOrEmpty(b)) throw new ArgumentNullException(nameof(b));
if(string.IsNullOrEmpty(c)) throw new ArgumentNullException(nameof(c));
var requestInfo = new RequestInformation {
HttpMethod = Method.GET,
UrlTemplate = UrlTemplate,
PathParameters = PathParameters,
};
applyUserConfig(requestInfo, config);
return requestInfo;
} library (in abstractions): public async void applyUserConfig(RequestInfo requestInfo, Action<RequestConfig> config) {
if (config != null) {
var requestConfig = new RequestConfig();
config.Invoke(requestConfig);
requestInfo.AddQueryParameters(requestConfig.QueryParameters);
requestInfo.AddRequestOptions(requestConfig.Options);
requestInfo.AddHeaders(requestConfig.Headers);
}
if (!requestInfo.Headers.ContainsKey("Accept")) {
requestInfo.Headers.Add("Accept", "application/json");
}
if (!requestInfo.Headers.ContainsKey("Content-Type")) {
requestInfo.SetContentFromScalar(RequestAdapter, "", b);
}
} wdyt? |
oh, yes, if we go and refactor the entire thing, adding a |
Adding a try add method: We expect users to update dependencies to what the kiota info command indicates as they refresh clients. So if we ship a generation change that requires additional API surface in kiota abstractions, it's not considered a breaking change. Apply user configuration method: that's a nice idea. The only issues with that:
For all those reasons, we had originally left this as generated code instead of wrapping everything in the abstractions. |
Thanks for the context @baywet , make sense. |
I need to get microsoft/kiota-abstractions-php#91 merged and released to finish this one 😓 |
I just did release 0.8.4 a few minutes ago, you should be good to go. Let me know if you need anything else. |
This is now ready for review (IT tests are passing on my fork). |
@andreaTP I think we're almost there. Can you add a changelog entry please? |
sure done 👍 |
Fixes #3378 for Java and C# as a start, if we agree that this is the desired encoding I'll review and propagate the changes to the rest of the languages.
This is a sample result for C#: