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

[Snippet Generation] Inconsistency in snippet generation when creating a Graph client #1577

Closed
maisarissi opened this issue May 30, 2023 · 10 comments
Assignees
Labels
area: SDKs area: snippets-generation Describes a required fix to the snippet generator status: needs-discussion An issue that requires more discussion internally before resolving

Comments

@maisarissi
Copy link

maisarissi commented May 30, 2023

In the API reference documentation, depending on the language you are looking at, we have different initialization of the client.
For example, looking at List user, C# and PHP initialize graph client with "request adapter", Java and JavaScript with authProvider and Go with token credentials.

I received question from folks trying to understand what is request adapter and believe this is causing unnecessary confusion.

We should have consistency among languages and would vote for token credential approach.

Would love to hear from others @isvargasmsft @sebastienlevert @CarolKigoonya

@sebastienlevert
Copy link

Is the link shared the right one you wanted to share? I see a link to Kiota?

@isvargasmsft
Copy link
Member

https://learn.microsoft.com/en-us/graph/api/user-list?view=graph-rest-1.0&tabs=http

I believe this is the right link. I see your point, Maisa, great catch. This is the type of issues that profoundly affect onboarding.

@sebastienlevert
Copy link

I agree we should have a unique way to drive how we think about auth. I believe the easiest way would be via a Credential. This simplifies a lot of what we do. So it's at the snippet that we generate this adapter... Is it really "required" or this should be an include outside the snippet? If I already have my app running, I probably don't need that and the copy feature now copies more than what I need...

@isvargasmsft
Copy link
Member

isvargasmsft commented May 31, 2023

I believe the snippet should only have the includes/import/use (depending on the language) and the code to call the method. I don't believe we should include initialization stuff in the snippet. Details about initialization should go in other articles.

Now, we should have a separate discussion about adding includes/import/use lines. I understand for some languages this is not a simple task.

@maisarissi
Copy link
Author

Is the link shared the right one you wanted to share? I see a link to Kiota?

I copied the wrong link, just updated it.

I believe the snippet should only have the includes/import/use (depending on the language) and the code to call the method. I don't believe we should include initialization stuff in the snippet. Details about initialization should go in other articles.

I'm ok with not having the graph client initialization, my only point here is to have the same experience across SDKs, so if we agree on not have graph client initialization there, we need to remove that from all code snippets.

At the same time, for brand new people to Graph, having a graphClient.Something() can be a little bit fuzzy as you are not sure where the graphClient is coming from. So we could change a little bit the wording we are using in the sentence below the code snippets and have something like:

image

Now, we should have a separate discussion about adding includes/import/use lines. I understand for some languages this is not a simple task.

Imports are especially important for Go due to how the client library is generated (flattened strucuture). We can see an example in the following situation:

image

Not sure imports are necessary to other languages.

@isvargasmsft
Copy link
Member

PHP, Python and I believe JavaScript also need to specify the imports, otherwise the code wont work and devs will have to browse the service library manually to find the right path, assuming they know what they need. Not adding the imports to the snippet is just a frustrating experience.

I like the idea of adding a note to redirect people to introductory articles if they need to initialize.

@millicentachieng millicentachieng added area: snippets-generation Describes a required fix to the snippet generator area: SDKs status: needs-discussion An issue that requires more discussion internally before resolving labels Jul 3, 2023
@maisarissi
Copy link
Author

These inconsistencies are being addressed by @sebastienlevert spec on snippet generation. The doc covers how snippets should look like.

@darrelmiller
Copy link
Contributor

How can we make these snippets fit naturally into our ServiceAdapter class best practice and where should we provide that best practice guidance, and who is going to write it? :-)

@sebastienlevert
Copy link

I understand where you are coming for @darrelmiller but this is another concern IMO. I'm OK with revisiting these snippets if required when we get that best practice out. I feel the snippets should be as atomic as possible so the switch to the service adapter class should be transparent or minimal.

@andrueastman
Copy link
Member

Closing this for now given the available spec. This has been broken down into individual language issues below
CLI - #1935
Go - #1936
Python - #1937
PHP - #1938
Typescript - #1939
Powershell - #1940

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: SDKs area: snippets-generation Describes a required fix to the snippet generator status: needs-discussion An issue that requires more discussion internally before resolving
Projects
None yet
Development

No branches or pull requests

6 participants