Skip to content

[Python] Added Support for Validating and Converting Model Attributes - #11134

Merged
wing328 merged 17 commits into
OpenAPITools:masterfrom
Bandwidth-Sandbox:ckoegel-py-attributes
Mar 14, 2022
Merged

[Python] Added Support for Validating and Converting Model Attributes#11134
wing328 merged 17 commits into
OpenAPITools:masterfrom
Bandwidth-Sandbox:ckoegel-py-attributes

Conversation

@ckoegel

@ckoegel ckoegel commented Dec 15, 2021

Copy link
Copy Markdown
Contributor

Solves #11133

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/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 (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01) @arun-nalla (2019/11) @spacether (2019/11)

@ckoegel

ckoegel commented Dec 16, 2021

Copy link
Copy Markdown
Contributor Author

Forcing the generator to throw errors when trying to create undeclared attributes caused many errors in the existing tests. I'm still fairly new to python, so I'm not sure if this is a widely accepted practice or not, but if anyone has insight as to why this is done and/or what should be done about it in this project, please let me know!

@ckoegel
ckoegel marked this pull request as ready for review December 16, 2021 19:26
@ckoegel

ckoegel commented Dec 16, 2021

Copy link
Copy Markdown
Contributor Author

@spacether I noticed that a single test has failed. I don't believe this is related to the changes made in this PR, as the tests run when running ./mvnw clean package all pass and the build is successful. It seems as though there is a TypeError in the one test that fails. Is there an issue with something else that is known or is this most likely an unintended result of my changes?

@spacether

spacether commented Dec 16, 2021

Copy link
Copy Markdown
Contributor

@ckoegel more tests needed to be run. I just approved them. My intention is for users to not pass in spec named parameters into Model(args) like you are doing in this PR. Can you try what I suggested in your issue and respond there?

@spacether spacether left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please try using the different method that we have for using spec named inputs. If that works then this PR is not needed. If it does not work then we should fix that interface. Details are in the comment I left in your issue

@spacether

spacether commented Jan 4, 2022

Copy link
Copy Markdown
Contributor

As shown by comments in the ticket, spec property naming can be used by passing in the _spec_property_naming argument when instantiating models. We need that parameter to differentiate the two use cases where

  • python named properties are input
  • spec named properties are input

@ckoegel How about I close this PR because our code already supports Validating and Converting Model Attributes with python naming and spec naming?

@ckoegel

ckoegel commented Jan 6, 2022

Copy link
Copy Markdown
Contributor Author

PR has been updated to reflect the findings in #11133

@ckoegel
ckoegel requested a review from spacether January 10, 2022 17:42

@spacether spacether left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for your PR. This looks great!

@spacether

Copy link
Copy Markdown
Contributor

@ckoegel sorry for my delay on reviewing this PR. Could you regenerate the samples here?
CI shows that these files need to be updated:

	modified:   samples/openapi3/client/petstore/python/petstore_api/model/foo_object.py
	modified:   samples/server/petstore/python-aiohttp-srclayout/setup.py
	modified:   samples/server/petstore/python-aiohttp-srclayout/tests/conftest.py
	modified:   samples/server/petstore/python-aiohttp-srclayout/tox.ini

After that update is made I can merge it in.

@wing328 wing328 added this to the 6.0.0 milestone Mar 14, 2022
@wing328
wing328 merged commit 266de25 into OpenAPITools:master Mar 14, 2022
@ckoegel
ckoegel deleted the ckoegel-py-attributes branch July 11, 2023 18:16
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