-
Notifications
You must be signed in to change notification settings - Fork 184
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
All indexers should be in the format client.Repos.ByOwner()
instead of client.ReposByOwner()
#2528
Comments
should this be ByRepo or WithRepo? |
We're using with for methods (odata) parameters. And by for indexers replacements. Aligning would provide a consistent experience since they are all different kind of parameters at the end. Thoughts @sebastienlevert ? |
Can you share an example @baywet? |
A good example of that is
That in Go today will look like client.UsersById("id").ReminderViewWithStartDateTimeAndEndDateTime(date, date).Get(ctx) If we aligned By and with, we'd have something like this client.Users().WithId("id").ReminderViewWithStartDateTimeAndEndDateTime(date, date).Get(ctx) If we didn't client.Users().ById("id").ReminderViewWithStartDateTimeAndEndDateTime(date, date).Get(ctx) |
I'm almost done implementing the change in Kiota. We went from this response, err := client.UsersById("id").Messages().Get(context.Background(), nil) to this response, err := client.Users().WithUserId("id").Messages().Get(context.Background(), nil) My guess is that people might complain that "WithUserId" is too long. The reason it's like that is because the parameter in the path when we convert the API metadata is /users/{user-id}/messages.... So here is my question: are we happy with "WithUserId" |
i'm happy with the WithUserId and I agree we shouldn't shorten. |
I'm curious on the with vs. by... To me By seems more natural... "Getting a user by it's id" vs. "Getting a user with it's id"... Might be a language thing from my side though... I'd love other PMs to chime in @maisarissi @isvargasmsft @darrelmiller |
We had a discussion about that in our Go sync meeting. I don't have a strong opinion and I'm ok either way. Using "with" to keep consistency as pointed out by Vincent is a nice touch though: |
You sold it for me @maisarissi! Let's go with With and no shortening as this could definitely create confusion and collisions! |
Bringing this up again, I feel like with is common in SQL and data base type queries, on the API side, I see in python/javascript/php, mostly users by, Could this be language specific. How was this resolved. |
Thanks for the insight here. Right now we have "With" for everything (indexers, multiple parameters, etc...) So: You're suggesting we change it to
Which would be fine, it's a single constant in the code and a couple of unit tests. Thoughts @ddyett @maisarissi @darrelmiller ? |
After a long and heated debate about English with the team:
|
here is the PR for that last change #2564 |
Currently, in C# we do the following:
client.Repos["owner"]["repo"].Pulls.GetAsync();
And it gets translated to something like this in other languages:
client.ReposByOwner("owner").ByRepo("repo").Get();
Though, we believe it would be a better experience to have it within the Repos request builder vs. as part of the root of the clent like this:
client.Repos.ByOwner("owner").ByRepo("repo").Get();
This would be a breaking change if we implement this change.
The text was updated successfully, but these errors were encountered: