Skip to content

Conversation

@da1910
Copy link
Collaborator

@da1910 da1910 commented Jul 14, 2022

Closes #220

This PR adds support for Enums as proeprties of models, it resurrects the code that I excised when we did some refactoring, and makes it slightly clearer what's going on. As part of model deserialization, if the model has no properties then we check if it has an implementation for get_real_child_model(). If it does not then we assume that it is an enum and return the string value.

This is certainly not an ideal approach, we should look to make a further improvement to this in the future, but that will likely require a major release of downstream packages to use an updated client generator. For the moment this is a patch level fix that gets enums working properly.

One test in each direction was added, and they both pass with the changes.

@da1910 da1910 requested a review from Andy-Grigg July 14, 2022 16:41
@da1910 da1910 added the bug Something isn't working label Jul 14, 2022
@codecov
Copy link

codecov bot commented Jul 14, 2022

Codecov Report

Merging #221 (37649fd) into main (fec10be) will decrease coverage by 0.17%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main     #221      +/-   ##
==========================================
- Coverage   91.88%   91.71%   -0.18%     
==========================================
  Files           8        8              
  Lines         826      833       +7     
==========================================
+ Hits          759      764       +5     
- Misses         67       69       +2     
Impacted Files Coverage Δ
src/ansys/openapi/common/_api_client.py 97.17% <75.00%> (-0.75%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fec10be...37649fd. Read the comment docs.

Copy link
Contributor

@Andy-Grigg Andy-Grigg left a comment

Choose a reason for hiding this comment

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

Looks good, one note about adding a docstring.


def get_real_child_model(self, data: Dict) -> str:
def get_real_child_model(self, data: Union[Dict, str]) -> str:
raise NotImplementedError()
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth putting a note in here that this method will be overridden in the auto-generated class if the model has a discriminator.

@Andy-Grigg
Copy link
Contributor

Also confirmed this works against a real service with enums.

@da1910 da1910 merged commit ce7f5eb into main Jul 15, 2022
@da1910 da1910 deleted the fix/220-handle_enums branch July 15, 2022 09:11
da1910 added a commit that referenced this pull request Jul 15, 2022
* Add test case with enum property

* Add fix for issue

* Add docstring as per review comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enums are not supported

2 participants