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

Fix TypeError in extraports_* functions #41

Merged
merged 3 commits into from
Sep 30, 2014
Merged

Fix TypeError in extraports_* functions #41

merged 3 commits into from
Sep 30, 2014

Conversation

orf
Copy link
Contributor

@orf orf commented Sep 8, 2014

Sometimes there is no extraports tag. In this case a TypeError is raised, which is confusing. Either a proper exception (MissingExtraports or something) should be raised, or None returned. This patch does the latter.

Sometimes there is no extraports tag. In this case a TypeError is raised, which is confusing. Either a proper exception (MissingExtraports or something) should be raised, or None returned. This patch does the latter.
@savon-noir
Copy link
Owner

Hello,

thx for your contribution.

I have two comments:

  1. I personally don't like the idea of having multiple returns in one function unless it really improves readability
  2. the change proposed breaks the pep8 compat, see travis log: https://travis-ci.org/savon-noir/python-libnmap/jobs/34698826

Could you correct?

Thanks again for your contribution.

Ronald

@orf
Copy link
Contributor Author

orf commented Sep 11, 2014

Done, although in the case of extraports_state I think multiple returns do help readability - the alternative would be to have return {'state': _xtrports['state'], 'count': _xtrports['count']} if _xtraports is not None else None which is not as readable as the current patch. But it's your call.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling 7be7f59 on orf:patch-1 into d8cdc0a on savon-noir:master.

savon-noir added a commit that referenced this pull request Sep 30, 2014
Fix TypeError in extraports_* functions
@savon-noir savon-noir merged commit 6e932aa into savon-noir:master Sep 30, 2014
@savon-noir
Copy link
Owner

sorry for the delays. I was quite busy these days. thanks for your contribution! I need to review the code a bit and do some improvement. Then I'll release a new version on pypi with your contribution included.

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.

None yet

3 participants