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

Why are properties on Requestbuilders not defined as python properties? #2024

Closed
darrelmiller opened this issue Dec 3, 2022 · 5 comments · Fixed by #2068
Closed

Why are properties on Requestbuilders not defined as python properties? #2024

darrelmiller opened this issue Dec 3, 2022 · 5 comments · Fixed by #2068
Assignees
Labels
Milestone

Comments

@darrelmiller
Copy link
Member

e.g. Why do we do client.me().get() instead of client.me.get() ?

@baywet
Copy link
Member

baywet commented Dec 6, 2022

This language semantics support it https://www.geeksforgeeks.org/getter-and-setter-in-python/
And the default behaviour from the code dom is to use properties (so they are being replaced in the refiner).
My guess is that it's easier a miss-communication or a desire to align with indexer (which is being replaced by a method with a parameter).
But getting insights from Isaac and @samwelkanda would be interesting

@baywet baywet added this to the Kiota post-GA milestone Dec 6, 2022
@isvargasmsft isvargasmsft self-assigned this Dec 6, 2022
@samwelkanda
Copy link
Contributor

samwelkanda commented Dec 9, 2022

This is something we looked into during implementation of generation but ultimately decided not to for the following reasons.

  1. Python properties are designed to be the pythonic way of working with attributes and simplify the implementation of getter, setter and deleter methods. In our case, we want to perform actions on the same object on each subsequent call of the fluent API. Mutating objects(the request object in our case) via properties is not considered idiomatic python. Using methods for mutating objects is recommended instead.
  2. If we were to ignore the above, property chaining would only work for paths that do not support arguments e.g client.users.get(), client.drives.get(). This is because setting a property requires the syntax instance.property = value.

This means for paths that require arguments a user can't do this:

await client.users_by_id('USER_ID').messages_by_id('MESSAGE_ID').get()

and will have to do this instead:

user = client.users_by_id = 'USER_ID'
message = user.messages_by_id = 'MESSAGE_ID'
await message.get()

Not only is this not fluent, but introduces differences in API calls depending on the path. Using method chaining avoids the above pitfalls with the only overhead being the need for caller syntax () on every method call.

@samwelkanda
Copy link
Contributor

Also, unlike methods, property setters do not support multiple arguments. You'll have to pass a tuple or an array.

@isvargasmsft
Copy link
Contributor

@darrelmiller, please let us know if you have further questions on this matter.

@darrelmiller
Copy link
Member Author

@samwelkanda @isvargasmsft

Python properties are designed to be the pythonic way of working with attributes and simplify the implementation of getter, setter and deleter methods. In our case, we want to perform actions on the same object on each subsequent call of the fluent API. Mutating objects(the request object in our case) via properties is not considered idiomatic python. Using methods for mutating objects is recommended instead.

Properties on request builders do not mutate the underlying request object, nor do they return the same object that was called. They return a new request factory for the child path segment. Request chaining is considered pythonic as per the email I shared from Guido in the past.

It is true that when accessing an Item request factory we will need to make that a method in order to pass the parameter. If the language supports property accessors then I see no reason not to use them to avoid developers have to type redundant parentheses. If accessing the request builder requires a parameter, then use a method. If it does not, then use a property.

With this approach we should be able to implement:

    client.me.get()
    client.me.messages_by_id('MESSAGE_ID').get()
    client.users_by_id('USER_ID').messages_by_id('MESSAGE_ID').get()
    client.solutions.bookings.bookingbusinesses.get()
    client.solutions.bookings.bookingbusinesses_by_id("someid").get()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants