Skip to content
This repository has been archived by the owner on May 15, 2019. It is now read-only.

Update base._to_dict_leaf function to display lists #130

Merged
merged 1 commit into from
May 12, 2018

Conversation

tcaiazza
Copy link
Contributor

@tcaiazza tcaiazza commented May 7, 2018

In cases where the final leaf element is a list the to dict function was
not properly translating the lists.

The trunk vlans in the interface model and the ip addresses under the
systems host entries are two examples.

This change will set the value to the _list element if it is present
otherwise continue on with current behavior

This PR is in response to
#129

In cases where the final leaf element is a list the to dict function was
not properly translating the lists.

The trunk vlans in the interface model and the ip addresses under the
systems host entries are two examples.

This change will set the value to the `_list` element if it is present
otherwise continue on with current behavior
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 83.91% when pulling aee20f2 on tcaiazza:to_dict into 6412cfa on napalm-automation:develop.

@@ -378,6 +378,9 @@ def _to_dict_leaf(element, filter):
try:
value = ast.literal_eval(element.__repr__())
except Exception:
value = element.__repr__()
if hasattr(element, '_list'):
Copy link
Member

Choose a reason for hiding this comment

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

I think we should move this to the try block:

try:
    if hasattr(element, '_list'):
       value = element._list
    else:
       value = ast.literal_eval(element.__repr__())
except Exception:
...

@dbarrosop
Copy link
Member

With the exception of the tiny comment I made I think it looks sane :)

@dbarrosop
Copy link
Member

Might also be great to add a test if you have the time ;)

@tcaiazza
Copy link
Contributor Author

@dbarrosop These tests would go under test/unit/test_parser/leaf/ correct?

@dbarrosop
Copy link
Member

I would just add a new test similar to this:

https://github.com/napalm-automation/napalm-yang/blob/develop/test/unit/test_text_tree.py

Where you load an object (no need to parse a real config, just load_dict from a json should be fine) and then dumping it. That'd actually test that we can serlialize and deserialize lists.

@dbarrosop dbarrosop merged commit aee20f2 into napalm-automation:develop May 12, 2018
@dbarrosop
Copy link
Member

Ok, fixed it here: 3d4e600

Note that I will be releasing 0.1.0 next and this one will include py3 support and will make use of the upstream version of pyangbind 0.8.0. I'd recommend testing carefully :)

@tcaiazza
Copy link
Contributor Author

Thanks for the heads up on the py3 stuff. I've started testing, no issues so far.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants