Skip to content

Conversation

@yugangw-msft
Copy link
Contributor

@yugangw-msft yugangw-msft commented Apr 25, 2018

Say, to ignore the empty additional_properties, the CLI code will be updated to

  def prune_empty_additional_props(obj, property_name, value):
         from msrest.serialization import Model
         return not isinstance(obj, Model) or property_name != 'additional_properties' or value

  result = todict(result, prune_empty_additional_props)

The initial profile data shows only 4 more milliseconds to list 36 virtual machines with overall 2.45 seconds realtime, so the increase is very marginal.

@yugangw-msft
Copy link
Contributor Author

yugangw-msft commented Apr 25, 2018

//cc: @lmazuel @derekbekoe
This PR is for initial evaluation, so to get some sense for whether or not this is the right way

@derekbekoe
Copy link
Contributor

Looks good. I'd be fine to introduce this change.

@yugangw-msft
Copy link
Contributor Author

yugangw-msft commented Apr 26, 2018

@derekbekoe, I updated the callback a bit so to be invoked after each layer's conversion. This gives the caller a chance to see both the original object and the dictionary object. This would allow more custom logics while the knack can stay unchanged.
BTW the CI failed for irrelevant code under 2.7. Because I know you recently fixed some similar issues in CLI, maybe you can suggest what is needed to get CI green?

************* Module knack.introspection
I: 99, 0: Useless suppression of 'no-member' (useless-suppression)
I: 77, 0: Useless suppression of 'deprecated-method' (useless-suppression)
************* Module knack.testsdk.base
W:182, 8: Relative import 'six.moves.urllib_parse', should be 'six.moves' (relative-import)
I:182, 0: Useless suppression of 'import-error' (useless-suppression)

@derekbekoe
Copy link
Contributor

Taking a look at CI.

@derekbekoe
Copy link
Contributor

See #82

@yugangw-msft
Copy link
Contributor Author

@derekbekoe, can you please take a look and approve it if no other issues were found?

@lmazuel
Copy link
Member

lmazuel commented May 2, 2018

@yugangw-msft I don't get your code, could you show again your example updated? The example in your initial comment takes three arguments, so I guess it was a former version. Could you share what you mean now?
My concern is that your return the result of the post_processor, I would have expected the code to change it in place.

@yugangw-msft
Copy link
Contributor Author

Sorry, i should have updated. See my CLI PR Azure/azure-cli#6277.

    @classmethod
    def remove_additional_prop_layer(cls, obj, converted_dic):
        from msrest.serialization import Model
        if isinstance(obj, Model):
            # let us make sure this is the additional properties auto-generated by SDK
            if ('additionalProperties' in converted_dic and isinstance(obj.additional_properties, dict)):
                converted_dic.update(converted_dic.pop('additionalProperties'))
        return converted_dic

@yugangw-msft yugangw-msft merged commit c5f9af5 into microsoft:master May 2, 2018
@yugangw-msft yugangw-msft deleted the tidy branch May 2, 2018 23:43
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