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

[Python] Fix the post processing of string enums #20976

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

Conversation

PidgeyBE
Copy link

@PidgeyBE PidgeyBE commented Mar 26, 2025

Description

This PR aims to fix #16560 and is basically an extension of a previous fix (#18566).
As explained in the linked/previous PR, the x-enum-varnames property set in openapi specs is not respected for string enums (for the Python generator).
The previous PR fixed that for int enums. The aim of this PR is to extend the fix to str enums.

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. @cbornet (2017/09) @tomplus (2018/10) @krjakbrjak (2023/02) @fa0311 (2023/10) @multani (2023/10)

@wing328
Copy link
Member

wing328 commented Mar 26, 2025

thanks for the PR

what about adding a test schema with x-enum-varnames in modules/openapi-generator/src/test/resources/3_0/python/petstore-with-fake-endpoints-models-for-testing.yaml ?

@PidgeyBE
Copy link
Author

thanks for the PR

what about adding a test schema with x-enum-varnames in modules/openapi-generator/src/test/resources/3_0/python/petstore-with-fake-endpoints-models-for-testing.yaml ?

I wanted to do that, but it already exists: https://github.com/OpenAPITools/openapi-generator/blob/v7.12.0/modules/openapi-generator/src/test/resources/3_0/python/petstore-with-fake-endpoints-models-for-testing.yaml#L2130

@wing328
Copy link
Member

wing328 commented Mar 27, 2025

The previous PR fixed that for int enums. The aim of this PR is to extend the fix to str enums.

given that this is a fix for the str enums, should there be some changes in the python samples?

@PidgeyBE
Copy link
Author

given that this is a fix for the str enums, should there be some changes in the python samples?

Alrighty, yeah I was a bit confused cause in #18566 the python samples were also not really clearly updated.
So I fixed both cases and regenerated the python samples to visualize the effect of the fix of the previous MR and this MR.

@wing328
Copy link
Member

wing328 commented Mar 27, 2025

@PidgeyBE thanks. can you review the build failure when you've time?

@PidgeyBE
Copy link
Author

@PidgeyBE thanks. can you review the build failure when you've time?

@wing328 Yes definitely, I looked into them and I can reproduce the issue on my dev pc, but I get the same issue on master branch... Also I don't see how my changes could affect that single test... :/
Is it possible these tests are somehow broken already?

@PidgeyBE
Copy link
Author

PidgeyBE commented Mar 27, 2025

@wing328 I've fixed the tests in 7a3a636, although I dont understand how they worked before...
I think in current master they are using the public https://petstore.swagger.io/ API, which is also visible in the logs:

self = <petstore_api.rest.RESTClientObject object at 0x7f5ae942f2b0>
method = 'DELETE', url = '[http://petstore.swagger.io:80/v2/pet/2050001115](http://petstore.swagger.io/v2/pet/2050001115)'

https://github.com/OpenAPITools/openapi-generator/actions/runs/14105031212/job/39517324941

So I guess https://petstore.swagger.io/ changed a bit or broke or ...

Should be good now

//edit: Well, seems like CircleCI is not able to checkout the code any more... But I can't help much with that I'm afraid 😬
//edit2: Seems like the CircleCI issue magically resolved itself

@wing328
Copy link
Member

wing328 commented Mar 30, 2025

I think in current master they are using the public https://petstore.swagger.io/ API, which is also visible in the logs:

should be using a local petstore server: https://github.com/OpenAPITools/openapi-generator/actions/runs/14125929740/workflow#L27-L33

@PidgeyBE
Copy link
Author

I think in current master they are using the public https://petstore.swagger.io/ API, which is also visible in the logs:

should be using a local petstore server: https://github.com/OpenAPITools/openapi-generator/actions/runs/14125929740/workflow#L27-L33

Yeah it should, but it didn't 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG][PYTHON][PLUSS OTHERS] Enum variable names not update in all python converters
2 participants