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

HeadersCollection usability #208

Open
andreaTP opened this issue Jan 2, 2024 · 10 comments
Open

HeadersCollection usability #208

andreaTP opened this issue Jan 2, 2024 · 10 comments
Assignees
Labels
enhancement New feature or request help wanted Good candidate for a pull request Standard GitHub label Issue caused by core project dependency modules or library

Comments

@andreaTP
Copy link
Contributor

andreaTP commented Jan 2, 2024

As demonstrated in microsoft/kiota#3962 I think we are facing a usability regression.
I don't have a strong opinion on where to go from here but either:

  • an extra constructor
  • more methods overload, e.g.get_all that accepts also a plain Dict, or similar.

cc. @baywet

@baywet baywet added enhancement New feature or request Standard GitHub label Issue caused by core project dependency modules or library labels Jan 8, 2024
@baywet
Copy link
Member

baywet commented Jan 8, 2024

Thanks for reporting this, is this something you'd be willing to take on?

@andreaTP
Copy link
Contributor Author

andreaTP commented Jan 8, 2024

is this something you'd be willing to take on?

It's easy enough, yes, I can pick it up, what would be the preferred solution?

@baywet
Copy link
Member

baywet commented Jan 8, 2024

We'd need something that feels "natural" in python.
For example in dotnet, because it implements the IDictionary interface the collection can be initialized with the dedicated notation:

new RequestHeaders {
   {"foo":  "bar"}
};

which feels natural to a dotnet developer

In Go, that's not supported, so we have two constructors (one empty, one that accepts a map)

It needs to feel like a map/dictionary API wise.

@andreaTP
Copy link
Contributor Author

andreaTP commented Jan 9, 2024

I have been taking a closer look at this, and ... I'm not sure 😢 .

Having headers as a publicly exposed field of RequestInformation makes any change I'm attempting a breaking change and, since Python is GA, we would need to wait for version 2 to introduce it.

Other than that, HeadersCollection should, probably, not be exposed to the users at all and this change requires again breaking changes to the API.

Looking for guidance on how to proceed here, or a Python expert that can show me the way 🙂 .

@baywet
Copy link
Member

baywet commented Jan 9, 2024

From you initial post it sounded like all the changes you wanted to make were additional.
Can you outline what changes might be breaking?

@andreaTP
Copy link
Contributor Author

andreaTP commented Jan 9, 2024

I thought that the changes were only additional ... but when attempting to implement it I found out that either we duplicate the headers field or we have to take into account the two types everywhere with a great maintenance burden.

In either case, since the headers field is used (and mutated) in this code-base and by the user this is causing great confusion.

@baywet
Copy link
Member

baywet commented Jan 9, 2024

Could we imagine having:

  • a private backing field that's a header collection
  • a getter and setter that are union type of header collection and map/dictionary
  • when the setter is called, type test the value being set, and if it's a maps, convert it to the collection
  • when the getter is called, always return the collection. (to avoid a breaking change)

Thoughts?

@andreaTP
Copy link
Contributor Author

andreaTP commented Jan 9, 2024

Your proposal makes a lot of sense coming from Java and C#, although I notice that some different tradeoffs are applied in this codebase.

Also, your proposal is an API breaking change(we cannot remove a public field), how would we roll it out?

@baywet
Copy link
Member

baywet commented Jan 9, 2024

I don't think it's breaking if the getter and the setters are using the same name as the public field we have today. which seems possible through descriptors but I'm no python expert.

@samwelkanda what's your opinion on all that?

@sebastienlevert sebastienlevert added this to the Kiota v1.12 milestone Feb 1, 2024
@lampajr
Copy link

lampajr commented Mar 19, 2024

Hey folks,
Sorry to jump in but I was having troubles on how to correctly provide headers and query parameters in Python generated clients and after finding the proper way to inject those values I agree that having something that is dict oriented could simplify the workflow (and also reduce some boilerplate code) for users, especially those coming from Python where dictionaries are widely used.

Similar example with query params:

    query_params = TestRequestBuilder.TestRequestBuilderGetQueryParameters(limit=1, page=0)
    config = RequestConfiguration(query_parameters=query_params, headers=HeadersCollection())

could be simplified by directly providing the query params as dictionary

    config = RequestConfiguration(query_parameters={limit:1, page:0}, headers=HeadersCollection())

Just my2c here as user, hope they are valuable inputs :)

@andrueastman andrueastman modified the milestones: Kiota v1.13, Kiota v1.14 Apr 8, 2024
@fey101 fey101 removed this from the Kiota v1.14 milestone Apr 26, 2024
@baywet baywet added help wanted Good candidate for a pull request and removed Needs: Attention 👋 labels May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Good candidate for a pull request Standard GitHub label Issue caused by core project dependency modules or library
Projects
Status: Todo 📃
Development

No branches or pull requests

7 participants