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

Fixes functions with parameters in Go #1681

Merged
merged 1 commit into from
Aug 1, 2023
Merged

Fixes functions with parameters in Go #1681

merged 1 commit into from
Aug 1, 2023

Conversation

rkodev
Copy link
Contributor

@rkodev rkodev commented Jul 24, 2023

Overview

Fixes generation fluent path in go for functions with paramerts e.g https://learn.microsoft.com/en-us/graph/api/reportroot-getyammeractivitycounts?view=graph-rest-1.0&tabs=http

Current behaviour

graphClient.Reports().GetYammerActivityCounts(period='{period}')().Get(context.Background(), nil)

expected behaviour

period = '{period}'
graphClient.Reports().GetYammerActivityCountsWithPeriod(&period).Get(context.Background(), nil)

Testing Instructions

  • Unit tests

@rkodev rkodev requested a review from a team as a code owner July 24, 2023 15:15
@rkodev rkodev enabled auto-merge (squash) July 24, 2023 15:59
{

private static readonly Regex FunctionWithParams = new(@"^(\w+)\(([^)]+)\)$", RegexOptions.Compiled, TimeSpan.FromMilliseconds(200));
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this is here and not using the FunctionWithParameterRegex in line 19?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The need here was to extract the parameters used in the example

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've combined these 2 since the existing regex both identify a function syntax. The difference is the new regex splits the function into "name" + "params" and make it simpler to return function names.

if (!funcWithParams.Item1)
return x.Segment.ToFirstCharacterUpperCase() + "().";

var varName = funcWithParams.Item3.FirstOrDefault().Key;
Copy link
Member

Choose a reason for hiding this comment

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

Multiple parameters is a possiblity that is not catered for.
Example at https://learn.microsoft.com/en-us/graph/api/callrecords-callrecord-getpstncalls?view=graph-rest-1.0&tabs=http

return x.Segment.ToFirstCharacterUpperCase() + "().";
else
{
var funcWithParams = x.Segment.GetFunctionWithParameters();
Copy link
Member

Choose a reason for hiding this comment

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

I think all you need to do here is call x.Segment.IsFunctionWithParameters()(already defined) then look up the parameters and values in snippetCodeGraph.PathParameters as

  • Functions with multiple parameters could exists
  • The snippetCodeGraph.PathParameters in contains type information that you can use to know for scenarios where the parameter is a number/Date type
  • Casing is consistent with the metadata therefore generating names that are aligned with the generated SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this. One concern with the Path Parameter is we can have an example using 1 or 2 params instead of all the path parameters. Thats the reason I opted for extracting the parameters from the snippet. I however do agree that the path param will give better type information

Copy link
Member

Choose a reason for hiding this comment

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

If a path had 3 parameters, and the example would use only 2 parameters, that would mean that one of the parameters is optional or has a pre-defined default. In such a case i believe the metadata would either

At this point of the snippet generation, the example path would already have been matched to the path that is relevant to it (when the snippet model is built) and every path parameter (including the optional ones if matched with their defaults) would be needed to be generated as only one url template is present and a parameter must be passed/included in function name.

Using the http URL would generate a function that does not exist in the SDK in such a scenario as the default parameter would be missing/not passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only left concern would be is it a given than the function with params is always the last segment of a path? This approach ensures that the params retain the position in the segment

Copy link
Member

Choose a reason for hiding this comment

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

It won't always be the last segment. But that is catered for as the operation can be checked on the specific segment as we traverse through the segments in the paths. Yes?
With the key values, you could still lookup casing/types from the snippetCodeGraph.PathParameters as well.

Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

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

Latest updates break some tests in PHP/C#/Poweshell.

Any chance we can

@sonarcloud
Copy link

sonarcloud bot commented Jul 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

93.1% 93.1% Coverage
0.0% 0.0% Duplication

@rkodev rkodev merged commit 444408c into dev Aug 1, 2023
9 checks passed
@rkodev rkodev deleted the fix/go-fluent-api-with branch August 1, 2023 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants