kiota icon indicating copy to clipboard operation
kiota copied to clipboard

Format generated Go code to adhere to golang coding standards

Open firefart opened this issue 9 months ago • 10 comments

Golang expects generated code to be properly go fmt'ed (see https://github.com/golang/go/issues/73181#issuecomment-2780661979).

This PR tries to implement the changes so the code will be compatible and go fmt will not change anything in the generated code.

Changes:

  • indent using tabs instead of spaces
  • when there is only one return argument, omit the parentheses
  • removes trailing spaces on comments without a content
  • fix import ordering
  • correctly indent case statements inside a switch

Test: To test we will create a test client. Next go to the golangtest directory, init a git repo, and run go fmt to see the differences

rm -rf ./golangtest/ && cp -r ./it/go golangtest && dotnet build && ./src/kiota/bin/Debug/net9.0/kiota generate --openapi https://aka.ms/graph/v1.0/openapi.yaml --output ./golangtest/client --language go --exclude-backward-compatible --clean-output -i '/groups/getByIds#POST' -n 'integrationtest/client'
cd golangtest
git init
git add .
go fmt ./...
git diff

Ideally the git diff will show no changes

Still to do

  • trailing spaces on setter methods
  • some missing spaces between declarations

firefart avatar Apr 06 '25 10:04 firefart

@baywet this is almost fnished but I'm struggling with one of the last pieces. There is a trailing space on all setter methods in interface declarations (Setxxxx(value xxx)). I can't find the code where this property is introduced, do you happen to know where to look exactly for this?

Example of an interface:

@@ -434,15 +435,15 @@
type AccessPackageable interface {
        GetIsHidden() *bool
        GetModifiedDateTime() *i336074805fc853987abe6f7fe3ad97a6a6f3077a16391fec744f671a015fbd7e.Time
        GetResourceRoleScopes() []AccessPackageResourceRoleScopeable
-       SetAccessPackagesIncompatibleWith(value []AccessPackageable) 
-       SetAssignmentPolicies(value []AccessPackageAssignmentPolicyable) 
-       SetCatalog(value AccessPackageCatalogable) 
-       SetCreatedDateTime(value *i336074805fc853987abe6f7fe3ad97a6a6f3077a16391fec744f671a015fbd7e.Time) 
-       SetDescription(value *string) 
-       SetDisplayName(value *string) 
-       SetIncompatibleAccessPackages(value []AccessPackageable) 
-       SetIncompatibleGroups(value []Groupable) 
-       SetIsHidden(value *bool) 
-       SetModifiedDateTime(value *i336074805fc853987abe6f7fe3ad97a6a6f3077a16391fec744f671a015fbd7e.Time) 
-       SetResourceRoleScopes(value []AccessPackageResourceRoleScopeable) 
+       SetAccessPackagesIncompatibleWith(value []AccessPackageable)
+       SetAssignmentPolicies(value []AccessPackageAssignmentPolicyable)
+       SetCatalog(value AccessPackageCatalogable)
+       SetCreatedDateTime(value *i336074805fc853987abe6f7fe3ad97a6a6f3077a16391fec744f671a015fbd7e.Time)
+       SetDescription(value *string)
+       SetDisplayName(value *string)
+       SetIncompatibleAccessPackages(value []AccessPackageable)
+       SetIncompatibleGroups(value []Groupable)
+       SetIsHidden(value *bool)
+       SetModifiedDateTime(value *i336074805fc853987abe6f7fe3ad97a6a6f3077a16391fec744f671a015fbd7e.Time)
+       SetResourceRoleScopes(value []AccessPackageResourceRoleScopeable)

firefart avatar Apr 10 '25 21:04 firefart

The first version is ready for review @baywet . There are still some minor issues which I was not able to fix as I did not find the code that generates the corresponding code. I will open an issue with the additional minor issues that need to be adressed

firefart avatar May 04 '25 21:05 firefart

@firefart thanks for the extended work on this. As for your question, have you looked here? https://github.com/microsoft/kiota/blob/e23ac20b0aef66454376deee667714f0ab50ca1f/src/Kiota.Builder/Writers/Go/CodePropertyWriter.cs#L31 It seems to me that if both the suffix and the return type are null or empty, we should not add the space.

baywet avatar May 13 '25 14:05 baywet

@firefart thanks for the extended work on this. As for your question, have you looked here?

https://github.com/microsoft/kiota/blob/e23ac20b0aef66454376deee667714f0ab50ca1f/src/Kiota.Builder/Writers/Go/CodePropertyWriter.cs#L31

It seems to me that if both the suffix and the return type are null or empty, we should not add the space.

@baywet thanks for the hint but this particular place it not the provblem. Added some checks, but the trailing spaces are still generated, so it must be elsewhere (but I suspect the same pattern)

firefart avatar May 16 '25 17:05 firefart

Thank you for the additional information.

First, I'm not seeing the trailing space behaviour from the main branch. Second, I spent more time looking into this, and it's actually here.

https://github.com/microsoft/kiota/blob/3491bb7f1943427e3c1e5fca5cef289a9ea8497a/src/Kiota.Builder/Writers/Go/CodeMethodWriter.cs#L476

Which breaks with the following conditional breakpoint.

code.IsOfKind(CodeMethodKind.Setter) && code.Parent is CodeInterface

Let me know if you have any additional comments or questions.

baywet avatar May 16 '25 18:05 baywet

@baywet thanks, that did the trick, the trailing whitespaces are now gone. I also tried to add an integration test to github actions to run go fmt ./... on the generated code and error out if there are formatted files. Could you please approve the github actions run so I can check if the test works? (running it locally is not really possible)

firefart avatar May 16 '25 20:05 firefart

@baywet fixed the unit tests, so would appreciate a new approval for all the tests.

PS: The used xunit version is really old, you should consider upgrading to xunit.v3. I needed to upgrade the local csproj files to this version so see the full strings from the Assert calls which is not possible in the current used version (I used XUNIT_PRINT_MAX_STRING_LENGTH=99999 dotnet test to test)

firefart avatar May 20 '25 06:05 firefart

The used xunit version is really old, you should consider upgrading to xunit.v3

Thanks for bringing this up. From reading the migration documentation, it seems there might be quite some work for this migration. If this is something you feel like you would like to submit a pull request for, I'd suggest starting a new issue.

baywet avatar May 20 '25 13:05 baywet

gentle reminder on this one @firefart

baywet avatar Jun 16 '25 18:06 baywet

@baywet sorry haven't found time yet to look into the tests, but hopefully by the end of the week to get this PR done

firefart avatar Jun 16 '25 21:06 firefart