Skip to content

[python] change Private attr to Class vars#16687

Merged
wing328 merged 7 commits intoOpenAPITools:masterfrom
fa0311:fix-type-error
Oct 1, 2023
Merged

[python] change Private attr to Class vars#16687
wing328 merged 7 commits intoOpenAPITools:masterfrom
fa0311:fix-type-error

Conversation

@fa0311
Copy link
Contributor

@fa0311 fa0311 commented Sep 29, 2023

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 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    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*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (upcoming 7.1.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.

Signed-off-by: ふぁ <yuki@yuki0311.com>
@fa0311 fa0311 marked this pull request as ready for review September 29, 2023 15:27
@fa0311
Copy link
Contributor Author

fa0311 commented Sep 29, 2023

fix: #16685 (comment)

No test exists for this fix.
To add a test, you must set disallowAdditionalPropertiesIfNotPresent to true.

disallowAdditionalPropertiesIfNotPresent: false

@multani
Copy link
Contributor

multani commented Sep 30, 2023

@fa0311 in #16655, you completely removed __properties, would it make sense to do the same instead?

I'm asking because using .defaults is a bit of a hack and it doesn't play well with type checking and I was looking for a way to remove it.

I wanted to replace it by cls.model_json_schema()["properties"].keys(), which would return the same, but if we can actually remove all of this, it would even be better.

@wing328
Copy link
Member

wing328 commented Sep 30, 2023

@fa0311 thanks for the PR. I've filed #16690 to add tests covering disallowAdditionalPropertiesIfNotPresent set to true and could repeat the issue.

@fa0311
Copy link
Contributor Author

fa0311 commented Sep 30, 2023

@multani I have come up with a good idea to fix this.
Replace cls.__properties.default with cls.model_fields.keys() and remove __properties.

(However, __discriminator_property_name.default and __discriminator_value_class_map.default remain unresolved.)

@multani
Copy link
Contributor

multani commented Sep 30, 2023

Replace cls.__properties.default with cls.model_fields.keys() and remove __properties.

This won't work out of the box: cls.model_field contains the names of the fields as they appear in the class definition, but not their "aliased" names.

This will break several tests, you can check.

On the other hand, cls.model_json_schema()["properties"].keys() contains actually the aliased names.

@multani
Copy link
Contributor

multani commented Sep 30, 2023

@fa0311
Copy link
Contributor Author

fa0311 commented Sep 30, 2023

@multani

@fa0311 Maybe this could actually help: https://docs.pydantic.dev/latest/concepts/models/#automatically-excluded-attributes

I was just looking at that myself.
It looks very good

Signed-off-by: ふぁ <yuki@yuki0311.com>
Signed-off-by: ふぁ <yuki@yuki0311.com>
@fa0311 fa0311 changed the title [python] fix TypeError [python] change Private attr to Class vars Sep 30, 2023
@fa0311
Copy link
Contributor Author

fa0311 commented Sep 30, 2023

A type hint is added to __properties and List becomes a reserved word.

Signed-off-by: ふぁ <yuki@yuki0311.com>
Signed-off-by: ふぁ <yuki@yuki0311.com>
Signed-off-by: ふぁ <yuki@yuki0311.com>
Copy link
Contributor

@multani multani left a comment

Choose a reason for hiding this comment

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

Yes, this is the good fix 🙇

@wing328
Copy link
Member

wing328 commented Oct 1, 2023

FYI. Samples updated via 738af45

@wing328 wing328 merged commit c6e9a4e into OpenAPITools:master Oct 1, 2023
@wing328
Copy link
Member

wing328 commented Oct 1, 2023

Tests added via #16690

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.

3 participants