Conversation
fed27a4 to
b16df2f
Compare
b16df2f to
ba07130
Compare
c7acaac to
276e900
Compare
f1e7b4f to
f70d6c1
Compare
| public queryParameters: Map<string, object> = new Map<string, object>(); //TODO: case insensitive | ||
| /** The Request Headers. */ | ||
| public headers: Map<string, string> = new Map<string, string>(); //TODO: case insensitive | ||
| private _middlewareOptions = new Map<string, MiddlewareOption>(); //TODO: case insensitive |
There was a problem hiding this comment.
Please use something like [MiddlewareControl](https://github.com/microsoftgraph/msgraph-sdk-javascript/blob/dev/src/middleware/MiddlewareControl.ts instead of using _middlewareOptions. Please move addMiddlewareOptions and getMiddlewareOptions options to the MiddlewareControl.
- A separate class for managing the properties as an object allows extensibility. A user can extend the MiddlewareControl class and flexibly customize their get and add middleware options or any property of the requestinfo class.
- We can observe Single Responsibility Principle if we neatly segregate some functionalities into specific classes.
- Avoids bulking up the RequestInfo class with too many methods.
There was a problem hiding this comment.
Yeah I was looking at the dotnet sdk which uses options but have uses control too. I'm not sure why we have a difference between languages today. @darrelmiller any guidance on options or control?
https://www.github.com/microsoftgraph/msgraph-sdk-dotnet-core/tree/dev/src%2FMicrosoft.Graph.Core%2FRequests%2FMiddleware%2FOptions%2FIMiddlewareOption.cs
We're attaching the options to request info to avoid passing multiple parameters between the layers. Request info is already being passed between layers. And the options or controls are related to the request info, which is why it felt natural to add them there. I'm not sure how you'd do it differently without having to pass additional parameters to the http core service?
There was a problem hiding this comment.
Both JS and .NET use the term Options. It looks like JS has a container for for middlewareOptions. .NET just uses a dictionary. I'm not sure why JS has the extra layer of indirection. Part of the confusion might be because the original requirement document talked about middleware controls. That notion got renamed to middleware options during implementation.
There was a problem hiding this comment.
Thanks for the context here. So I'm guessing we're sticking with options then.
|
What is the reason to use predefined names such as fetch, okhttp as is and not a custom name like kiota-fetch or kiota-httpclient? My concern is with
|
|
@nikithauc I'm not sure what you are referring to? The package name? A specific class? |
6c08c5f to
4f0bcb8
Compare
| using System.Net.Http; | ||
| using Microsoft.Kiota.Abstractions; | ||
|
|
||
| namespace Microsoft.Kiota.Http.HttpClient { |
There was a problem hiding this comment.
What is the likelihood of this namespace being used in the same context as System.Net.Http.HttpClient? If they show up in the same context, the naming collision can be confusing for the customers. Could we rename the last part or drop it altogether to bump the namespace one level up to Microsoft.Kiota.Http?
There was a problem hiding this comment.
@baywet, I think @nikithauc is referring to this.
There was a problem hiding this comment.
aaah we had a long naming debate with @darrelmiller a long time ago.
I wanted to name this Microsoft.Kiota.Transport.Http leaving room for other transport implementations potentially in the future. Thoughts on that name? And for typescript we'd have something like @microsoft/kiota-transport-http.
There was a problem hiding this comment.
What are transport implementations?
There was a problem hiding this comment.
well one of them is HTTP of course, another one could be gRPC, etc...
There was a problem hiding this comment.
I think another common thing between HTTP and gRPC is that they are both protocols. So we can also have Microsoft.Kiota.Protocol.Http or Microsoft.Kiota.Protocols.Http as the namespace alternatives. I am OK with Microsoft.Kiota.Transport.Http too.
There was a problem hiding this comment.
I don't think transport is correct here. Protocol is more accurate. For example, if we introduced WebSockets, we'd have:
Microsoft.Kiota.Protocol.Http
Microsoft.Kiota.Protocol.WebSocket
They use the same transport.
There was a problem hiding this comment.
Microsoft.Kiota.Protocol.Http
Microsoft.Kiota.Protocol.WebSocket
The namespace looks meaningful.
Hypertext Transfer Protocol (HTTP) is an application-layer protocol for transmitting hypermedia documents, such as HTML. It was designed for communication between web browsers and web servers, but it can also be used for other purposes.
There was a problem hiding this comment.
yep, protocol works for me as well. As this is a discussion on existing naming pattern, and as I'd like Darrel's "blessing" before changing the naming, I'm going to create a separate issue for that. Thanks for all the great suggestions!
- adds unit tests for options parameter
4f0bcb8 to
1d088a6
Compare
|
Kudos, SonarCloud Quality Gate passed! |








Summary
fixes #120
Generation Result
microsoft/kiota-samples#146