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

[Rust] fix #20198, cleanup generated code #20203

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xMAC94x
Copy link

@xMAC94x xMAC94x commented Nov 28, 2024

This change adjust the generated rust code to be easier readable thanks to removing the local_var_ prefix.
It also gets rid of the naming problems in a whole because it gets rid of the "unbox" in the template.

The change will modify code generated with openapi-generator but should not interact with the generated interfaces, thus I think its no breaking change.

No extra tests where added and this change should validate if CI passes the integration tests.

[RUST] @frol @farcaller @richardwhiuk @paladinzh @jacob-pro

see #20198

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package || exit
    ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
    ./bin/utils/export_docs_generators.sh || exit
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

get rid of `local_var_` in order to make the code more readable.
Get rid of the "unbox" of parameters at function start, so that we dont have any problem with name clashes.
@xMAC94x
Copy link
Author

xMAC94x commented Nov 29, 2024

@ThomasVille I saw your latest PR-merge and rebased this branch to resolve the merge conflicts :)

@wing328
Copy link
Member

wing328 commented Nov 30, 2024

@xMAC94x thanks for the RP

the local_var_ prefix is to avoid naming collision with names defined in the spec (e.g. property/parameter/model names, etc)

we can make it customizable with an option and you can set it to an empty string. would it meet your requirement?

@xMAC94x
Copy link
Author

xMAC94x commented Dec 2, 2024

@wing328

Thank you for the answer.
So a prefix, like local_var_ is only a workaround, as any property called local_var_<x> will now collide. Yes no one names their properties local_var_ so it will eventually work "good enough".

I see 2 alternatives:

This PR implements number 2. It removes the variable unboxing and with it, it should also remove the potential for ANY name collisions that local_var_ prevented. (Note: It's still possible to create a name collision if you have useSingleRequestParameter = false and name your parameter something like configuration . That was possible before this PR and will still be possible with this PR)
(Note2: One could also combine the 2 alternatives above, maybe even do alternative1 in all languages, this is my first PR though, I wanted to keep things small and mergeable).

@wing328 can you double-check and confirm that with this PR no name clashes with local variables are possible or show me an edge case that I missed?

(edit: spelling)

@wing328
Copy link
Member

wing328 commented Dec 2, 2024

I've filed #20219

can you please give it a try to see if it meets your requirements?

e.g. in CLI, add --additional-properties varPrefix=""

@wing328
Copy link
Member

wing328 commented Dec 2, 2024

another way to avoid parameter name collision is to use the parameterNameMappings option: https://github.com/openapitools/openapi-generator/blob/master/docs/customization.md#name-mapping

@xMAC94x
Copy link
Author

xMAC94x commented Dec 2, 2024

another way to avoid parameter name collision is to use the parameterNameMappings option: https://github.com/openapitools/openapi-generator/blob/master/docs/customization.md#name-mapping

Thanks, that probably even better than the alternative1 from above, as here keep the same by default and the user can fine-granular decide what they need.

I've filed #20219

can you please give it a try to see if it meets your requirements?

e.g. in CLI, add --additional-properties varPrefix=""

I prob posted my comment in the same time you created that PR :D
Can you recheck, AFAIK this PR should prevent all collisions that a variable prefix catches, thus we could get completely get rid of both: the collisions, and the "weird" looking names. A dynamic prefix for the variables should not be necessary.

@wing328
Copy link
Member

wing328 commented Dec 2, 2024

when you've time, can you please PM me via Slack to further discuss this?

https://join.slack.com/t/openapi-generator/shared_invite/zt-2uoef5v0g-XGwo8~2oJ3EoziDSO1CmdQ

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.

2 participants