update base model and add as_dict support for base model #2027
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tadelesh
commented
Jul 28, 2023
| _content = resource | ||
| else: | ||
| _content = json.dumps(resource, cls=AzureJSONEncoder) # type: ignore | ||
| _content = json.dumps(resource, cls=AzureJSONEncoder, exclude_readonly=True, exclude_none=False) # type: ignore |
Member
Author
There was a problem hiding this comment.
I don't think we should replace it with as_dict bc:
- body could be non model
- body could be a
MutableMapping[str, Any]that mix simpledictandModel json.dumps(Model.as_dict())will go through all models' attributes twice, it is not effiecient
Member
Author
There was a problem hiding this comment.
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
iscai-msft
reviewed
Jul 31, 2023
| create_body_call = ( | ||
| f"_{body_kwarg_name} = json.dumps({body_param.client_name}, " | ||
| "cls=AzureJSONEncoder) # type: ignore" | ||
| "cls=AzureJSONEncoder, exclude_readonly=True,\n exclude_none=False) # type: ignore" |
Contributor
There was a problem hiding this comment.
we exclude_none True right now, it's just that it's currently handled in the JSON encoder. I feel it'll be clearer if we just pass in True here
Member
Author
There was a problem hiding this comment.
I'm a little confused. Let me give an example that you could help to correct:
class TestModel(Model):
prop1: str
prop2: str
prop3: str
model = TestModel({"prop1": None, "prop2": NULL})
json.dumps(model, cls=AzureJSONEncoder, exclude_none=True) # the result should be {"porp2": null}
json.dumps(model, cls=AzureJSONEncoder, exclude_none=False) # test result should be {"prop1": null, "prop2": null}
packages/autorest.python/autorest/codegen/templates/model_base.py.jinja2
Outdated
Show resolved
Hide resolved
| models.update({ | ||
| k: v | ||
| for k, v in sys.modules[module_end].__dict__.items() | ||
| if isinstance(v, (type, typing._GenericAlias)) # type: ignore |
Contributor
There was a problem hiding this comment.
what is this added code for? I don't know if we should access a private model in the typing namespace
Member
Author
There was a problem hiding this comment.
It is for type alias like:
union MyNamedUnion {
one: Model1,
two: Model2,
}
packages/autorest.python/autorest/codegen/templates/model_base.py.jinja2
Outdated
Show resolved
Hide resolved
packages/autorest.python/autorest/codegen/templates/model_base.py.jinja2
Outdated
Show resolved
Hide resolved
packages/autorest.python/autorest/codegen/templates/model_base.py.jinja2
Outdated
Show resolved
Hide resolved
tadelesh
changed the title
iscai-msft
approved these changes
Aug 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolve #1990
model base change and test is here: Azure/azure-sdk-for-python#31028
also, fix #2034, #2035, #2036