Skip to content

Conversation

@lmazuel
Copy link
Member

@lmazuel lmazuel commented Jan 31, 2017

Didn't test with Autorest testsuite yet, but since this is JSON-like syntax, I'm pretty sure there is no test in Autorest anyway.

@lmazuel lmazuel requested a review from annatisch January 31, 2017 01:03
@lmazuel
Copy link
Member Author

lmazuel commented Jan 31, 2017

Tested by @vishrutshah on his Monitor issue and all SDK tests ok.

@lmazuel
Copy link
Member Author

lmazuel commented Feb 1, 2017

@annatisch I dig inside the msrest code and I realize I re-wrote _classify but calling it _infer_type. I will rework on this to simplify code and avoid duplication.

@annatisch
Copy link
Member

@lmazuel - yes I thought there might be some duplication there - I was about to compare them myself :)

@lmazuel
Copy link
Member Author

lmazuel commented Feb 1, 2017

@annatisch if you have the time, I pushed an update that should still fix the serialization + the deserialisation at the time (rewriting classify to include both fix). I didn't launch Autorest tests or SDKs yet.

Copy link
Member

@annatisch annatisch 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 to me - if the autorest/SDK tests pass should be good to go :)

@annatisch
Copy link
Member

@lmazuel - yup looks like it! :) Yay!

@lmazuel
Copy link
Member Author

lmazuel commented Feb 6, 2017

@annatisch last commit was debugged directly with @vishrutshah in peer programming. This allows to accept as key in the dict (serialisation) both the JSON key name and the Python key name.
Let's say RestAPI uses "awesomeName", so Python will use "awesome_name". Now for subtyping, both syntax are correct in a dict.
This allows to figure out subtyping even from a dict (serialisation) and from a JSON (deserialisation) even if the key are not expected to be the same. Also, it's been a long time I think about accepting both keys syntax in dict serialization (to simplify a copy/paste from a RestAPI example), so I'm shocked and don't consider this a hack.
Unittest to be added before merging this PR.

@lmazuel lmazuel merged commit 78d9329 into master Feb 10, 2017
@lmazuel lmazuel deleted the polymorphic_from_json branch May 23, 2017 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants