Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lxml text retrieval (find_txt) shouldn't fail if no text element available #1242

Merged
merged 1 commit into from
Jun 30, 2020

Conversation

hellt
Copy link
Contributor

@hellt hellt commented Jun 22, 2020

Hi maintainers,
I propose to enhance the find_txt function so it won't log an error when it tries to get the text out of the element that has no text element present.

Consider the following example:

import lxml
from napalm.base.helpers import find_txt

s = """
<port>
    <port-id>1/1/c5/1</port-id>
    <oper-state>up</oper-state>
    <system-enabled-capabilities></system-enabled-capabilities>
</port>
"""

e = lxml.etree.fromstring(s)

print(
    "element with text value:", find_txt(e, "oper-state", "default_value",),
)

print(
    "element with no text value:",
    find_txt(e, "system-enabled-capabilities", "default_value",),
)

In the XML snippet above, the current find_txt behaviour will log an error if one tries to get the text out of the element with not text value (second print func):

# python run_orig_find_txt.py
element with text value: up
'NoneType' object has no attribute 'strip'
element with no text value: default_value

The proposed PR adds additional conditional branch to not try to get the text value from a None object. Which will result in a clean execution:

# python run_new_find_txt.py
element with text value: up
element with no text value: default_value

@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling af3dbee on hellt:find_txt into 3e676cb on napalm-automation:develop.

Copy link
Member

@mirceaulinic mirceaulinic left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @hellt!

@mirceaulinic mirceaulinic merged commit 7989e67 into napalm-automation:develop Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants