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

Feature/http language support #5778

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

Conversation

koros
Copy link
Contributor

@koros koros commented Nov 14, 2024

Add support for HTTP snippet generation as a language option.

koros added 30 commits October 9, 2024 16:10
@koros koros marked this pull request as ready for review November 27, 2024 17:35
@koros koros requested a review from a team as a code owner November 27, 2024 17:35

public HttpPathSegmenterTests()
{
segmenter = new HttpPathSegmenter("D:\\source\\repos\\kiota-sample", "client");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace the hard-coded path here by temp/random path. (search for GetTempPath in the codebase for examples)

namespace Kiota.Builder.Writers.Http;
public class CodePropertyWriter(HttpConventionService conventionService) : BaseElementWriter<CodeProperty, HttpConventionService>(conventionService)
{
public override void WriteCodeElement(CodeProperty codeElement, LanguageWriter writer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is a NO-OP, why don't we use the refiner to remove properties from the dom instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed all non-RequestBuilder types from the generated code DOM. However, the remaining RequestBuilder classes can still contain properties, functions, and other elements. If I were to remove the writer stubs entirely, it would result in an exception when the writer encounters these elements and attempts to delegate their writing to the language-specific writers, which would be null in this case.

{
public override void WriteCodeElement(CodeMethod codeElement, LanguageWriter writer)
{
ArgumentNullException.ThrowIfNull(codeElement);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment about the NO-OP

namespace Kiota.Builder.Writers.Http;
public class CodeEnumWriter(HttpConventionService conventionService) : BaseElementWriter<CodeEnum, HttpConventionService>(conventionService)
{
public override void WriteCodeElement(CodeEnum codeElement, LanguageWriter writer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment about the NO-OP

var queryParameterClasses = requestBuilderClass
.GetChildElements(true)
.OfType<CodeClass>()
.Where(element => element.IsOfKind(CodeClassKind.QueryParameters))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.Where(element => element.IsOfKind(CodeClassKind.QueryParameters))
.Where(static element => element.IsOfKind(CodeClassKind.QueryParameters))

Comment on lines 291 to 295
writer.WriteLine("{", includeIndent: false);
writer.IncreaseIndent();
WriteProperties(propClass, writer);
writer.DecreaseIndent();
writer.Write("}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use start and close block

/// <param name="writer">The language writer to write the properties to.</param>
private static void WriteProperties(CodeClass requestBodyClass, LanguageWriter writer)
{
var properties = requestBodyClass.Properties.Where(prop => prop.IsOfKind(CodePropertyKind.Custom)).ToList();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var properties = requestBodyClass.Properties.Where(prop => prop.IsOfKind(CodePropertyKind.Custom)).ToList();
var properties = requestBodyClass.Properties.Where(static prop => prop.IsOfKind(CodePropertyKind.Custom)).ToArray();

private static void WriteProperties(CodeClass requestBodyClass, LanguageWriter writer)
{
var properties = requestBodyClass.Properties.Where(prop => prop.IsOfKind(CodePropertyKind.Custom)).ToList();
for (int i = 0; i < properties.Count; i++)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we use a foreach instead?

/// <returns>The default value as a string.</returns>
private static string GetDefaultValueForProperty(CodeProperty codeProperty)
{
return codeProperty.Type.Name switch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be in the convention service

urlTemplateString = urlTemplateString.Trim('"').Replace("{+baseurl}", baseUrl, StringComparison.InvariantCultureIgnoreCase);

// Build RequestInformation using the URL
var requestInformation = new RequestInformation()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need to do any of that instead of simply grabbing the URI template from the execution method?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe he's using the RequestInformation resolve a valid url from the provided query and path parameters. So that the placeholders can be filled rather than have placeholders(which is what the uritemplate has)

src/Kiota.Builder/Refiners/HttpRefiner.cs Outdated Show resolved Hide resolved
src/Kiota.Builder/Refiners/HttpRefiner.cs Outdated Show resolved Hide resolved
if (element is not CodeClass codeClass) return;

var parent = element.GetImmediateParentOfType<CodeNamespace>().Parent;
while (parent is not null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to call this from the refiner method directly as this would handle the traversal of the tree for you?
My concern here is that if two classes exist in the same namespace, you'll traverse them twice as you will be referencing to the same namespace and select all the children over again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example of how the traversal would look like?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, AddPathParameters is currently called from RemoveUnusedCodeElements.
Since RemoveUnusedCodeElements crawls the tree, it will go through each element in the DOM(line 90).

So, if you have two classes in the same namespace, each class may be called as a parameter for AddPathParameters meaning that you will get the same namespace twice(for each time you encounter the class) and enumerate ALL the children. (When you get to the parent of the namespace, I believe you will list all the children which were already listed in a previous iteration as they are part of the child elements of the previous parent)

Instead, you can simply add a method called directly in the RefineAsync (after removing the classes/methods you don't need). The method would simply check if the codeElement is a CodeMethod and the kind is CodeMethodKind.IndexerBackwardCompatibility then add the path parameters. And crawl the tree.

src/Kiota.Builder/Writers/HTTP/HttpConventionService.cs Outdated Show resolved Hide resolved
urlTemplateString = urlTemplateString.Trim('"').Replace("{+baseurl}", baseUrl, StringComparison.InvariantCultureIgnoreCase);

// Build RequestInformation using the URL
var requestInformation = new RequestInformation()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe he's using the RequestInformation resolve a valid url from the provided query and path parameters. So that the placeholders can be filled rather than have placeholders(which is what the uritemplate has)

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

Successfully merging this pull request may close these issues.

4 participants