-
Notifications
You must be signed in to change notification settings - Fork 214
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
base: main
Are you sure you want to change the base?
Conversation
|
||
public HttpPathSegmenterTests() | ||
{ | ||
segmenter = new HttpPathSegmenter("D:\\source\\repos\\kiota-sample", "client"); |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.Where(element => element.IsOfKind(CodeClassKind.QueryParameters)) | |
.Where(static element => element.IsOfKind(CodeClassKind.QueryParameters)) |
writer.WriteLine("{", includeIndent: false); | ||
writer.IncreaseIndent(); | ||
WriteProperties(propClass, writer); | ||
writer.DecreaseIndent(); | ||
writer.Write("}"); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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++) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
if (element is not CodeClass codeClass) return; | ||
|
||
var parent = element.GetImmediateParentOfType<CodeNamespace>().Parent; | ||
while (parent is not null) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
urlTemplateString = urlTemplateString.Trim('"').Replace("{+baseurl}", baseUrl, StringComparison.InvariantCultureIgnoreCase); | ||
|
||
// Build RequestInformation using the URL | ||
var requestInformation = new RequestInformation() |
There was a problem hiding this comment.
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)
Add support for HTTP snippet generation as a language option.