equinix-labs / metal-go

[Deprecated] Golang client for Equinix Metal
https://deploy.equinix.com/labs/equinix-sdk-go/
MIT License
3 stars 2 forks source link

Investigate using `reservedWordsMapping` to control acronym casing #112

Closed ctreatma closed 4 months ago

ctreatma commented 1 year ago

This SDK was recently updated to use openapi-generator v6.6.0. That version adds support for reservedWordsMapping in the Go generator: https://github.com/OpenAPITools/openapi-generator/pull/15083

We may be able to use reservedWordsMapping to standardize how certain acronyms (Bgp/BGP, Vrf/VRF, etc.) are capitalized in generated code.

Try adding a reserved words mapping to this SDK to see if that feature will enable us to standardize acronym capitalization without introducing unintended side effects.

displague commented 1 year ago

I ran with the following changes and found most methods and models updated, but there are still several cases of generated types not respecting the initialism (FindSomethingById, FindSomethingByIdRequest, FindSomethingById200Response).

diff --git a/Makefile b/Makefile
index a888e25..b418ce5 100644
--- a/Makefile
+++ b/Makefile
@@ -11,7 +11,7 @@ SPEC_PATCHED_DIR=spec/oas3.patched

 SPEC_FETCHED_FILE:=spec.fetched.json
 SPEC_PATCHED_FILE:=spec.patched.json
-OPENAPI_IMAGE_TAG=v6.6.0
+OPENAPI_IMAGE_TAG=v7.0.0
 OPENAPI_IMAGE=openapitools/openapi-generator-cli:${OPENAPI_IMAGE_TAG}
 GIT_ORG=equinix-labs
 GIT_REPO=metal-go
@@ -49,12 +49,25 @@ patch-post:
 clean:
        rm -rf $(PACKAGE_PREFIX)

+run-image:
+       $(OPENAPI_GENERATOR)
+
 gen:
        ${OPENAPI_GENERATOR} generate -g go \
                --package-name ${PACKAGE_MAJOR} \
                --http-user-agent "${GIT_REPO}/${PACKAGE_VERSION}" \
                -p packageVersion=${PACKAGE_VERSION} \
                -p isGoSubmodule=true \
+               --reserved-words-mappings Bgp=BGP \
+               --reserved-words-mappings Vrf=VRF \
+               --reserved-words-mappings Ip=IP \
+               --reserved-words-mappings ById=ByID \
+               --reserved-words-mappings Id=ID \
+               --reserved-words-mappings Totp=TOTP \
+               --reserved-words-mappings Otp=OTP \
+               --reserved-words-mappings Tfa=TFA \
+               --reserved-words-mappings Ssh=SSH \
+               --reserved-words-mappings Sms=SMS \
                -p disallowAdditionalPropertiesIfNotPresent=false \
                --git-user-id ${GIT_ORG} \
                --git-repo-id ${GIT_REPO}/${PACKAGE_PREFIX} \
displague commented 1 year ago

I feared Id=ID would be too aggressive, but that seems not to be the case in this project. In fact, it may be unnecessary based on upstream changes to follow Go initialisms recommendations. Nonetheless, the diff still included ById methods.

displague commented 1 year ago

I also see fields named: iD ☹️

displague commented 1 year ago

run-image: seems incomplete and may need to pass a ARGS:="" in. The intention was to make it easier to use the docker execution pattern for running commands like generate --help

ctreatma commented 4 months ago

Closing; while the acronym casing might not match our ideal, it nonetheless works. In our limited experimentation with openapi-generator options, none of them really provided the ideal output. If the Metal team wants to put more effort into changing acronym casing, they should do that in equinix/equinix-sdk-go.