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

[sebastienlevert]

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.

[ddyett]

should this be ByRepo or WithRepo?

[baywet]

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]

Can you share an example @baywet?

[baywet]

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]

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]

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

[sebastienlevert]

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

[maisarissi]

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]

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

[shemogumbe]

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]

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 ?

[baywet]

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]

here is the PR for that last change #2564