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

All indexers should be in the format client.Repos.ByOwner() instead of client.ReposByOwner() #2528

Closed
sebastienlevert opened this issue Apr 5, 2023 · 13 comments · Fixed by #2532 or #2564
Assignees
Labels
generator Issues or improvements relater to generation capabilities. Go javascript Pull requests that update Javascript code PHP Python Ruby TypeScript Pull requests that update Javascript code WIP
Milestone

Comments

@sebastienlevert
Copy link
Contributor

sebastienlevert commented Apr 5, 2023

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.

@sebastienlevert sebastienlevert added TypeScript Pull requests that update Javascript code Python PHP Go Ruby javascript Pull requests that update Javascript code generator Issues or improvements relater to generation capabilities. and removed Needs: Triage 🔍 labels Apr 5, 2023
@sebastienlevert sebastienlevert added this to the Kiota v1.2 milestone Apr 5, 2023
@ddyett
Copy link
Member

ddyett commented Apr 5, 2023

should this be ByRepo or WithRepo?

@baywet
Copy link
Member

baywet commented Apr 5, 2023

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 ?

@sebastienlevert
Copy link
Contributor Author

Can you share an example @baywet?

@baywet
Copy link
Member

baywet commented Apr 6, 2023

A good example of that is

GET /users/{id | userPrincipalName}/reminderView(startDateTime=startDateTime-value,endDateTime=endDateTime-value)

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)

@baywet
Copy link
Member

baywet commented Apr 6, 2023

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....
I could just take the last part, but truncating things like that in the past has led to collisions in the past.
We can't have the conversion process just produce /users/{id}/messages/{id} because now the url template won't work either.

So here is my question: are we happy with "WithUserId"

@ddyett
Copy link
Member

ddyett commented Apr 6, 2023

i'm happy with the WithUserId and I agree we shouldn't shorten.

@sebastienlevert
Copy link
Contributor Author

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

@baywet baywet self-assigned this Apr 10, 2023
@maisarissi
Copy link
Contributor

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:
**With**Id("id") and ReminderView**With**StartDateTimeAndEndDateTime.
Also, it was brought to my attention that with is more accurate from a grammatical perspective. For example, in the GitHub API it makes more sense to have client.Repos.WithOwner("owner").WithRepo("repo").Get(); then ByRepo.

@sebastienlevert
Copy link
Contributor Author

You sold it for me @maisarissi! Let's go with With and no shortening as this could definitely create confusion and collisions!

@shemogumbe
Copy link
Contributor

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.

@baywet
Copy link
Member

baywet commented Apr 13, 2023

Thanks for the insight here. Right now we have "With" for everything (indexers, multiple parameters, etc...)

So:
client.Users().WithId("id").CheckPermissionsWithRoleAndPath("role", "path").Get()

You're suggesting we change it to

client.Users().ById("id").CheckPermissionsByRoleAndPath("role", "path").Get()

Which would be fine, it's a single constant in the code and a couple of unit tests.
Besides being shorter, another benefit @sebastienlevert is that B comes before T so in the autocompletion those methods would come before the "ToXXXRequestInformation" ones.

Thoughts @ddyett @maisarissi @darrelmiller ?

@isvargasmsft isvargasmsft reopened this Apr 13, 2023
@baywet
Copy link
Member

baywet commented Apr 13, 2023

After a long and heated debate about English with the team:

  • By for single parameters (entire path segment is a single parameter)
  • With for multiple parameters (anything else)

@baywet
Copy link
Member

baywet commented Apr 14, 2023

here is the PR for that last change #2564

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generator Issues or improvements relater to generation capabilities. Go javascript Pull requests that update Javascript code PHP Python Ruby TypeScript Pull requests that update Javascript code WIP
Projects
Archived in project
6 participants