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

Conform to PEP8 #8

Merged
merged 2 commits into from
Jan 20, 2016
Merged

Conform to PEP8 #8

merged 2 commits into from
Jan 20, 2016

Conversation

super3
Copy link
Contributor

@super3 super3 commented Jan 19, 2016

Also rename vars shadowing built-ins.

Also rename vars shadowing built-ins
@yimingliu
Copy link
Owner

Thanks! I haven't had a chance to test it, but I've been meaning to PEP8-fy this module and just never had the time.

Could I get you to do two minor things?

  • In the new NatPMP class, the proto variable is hard-coded inline to 1 and 2. Could use the NATPMP_PROTOCOL_TCP and NATPMP_PROTOCOL_UDP constants already in the module to make it more readable.
  • I wish something netifaces existed back when I wrote this first, and would definitely have used it, since gateway detection was the most fragile part of the code. However, now I have legacy code deployed based on NATPMP.py, that doesn't rely on installing a third-party dependency. Can you do an ImportError exception check in get_gateway_addr to see netifaces can be imported, and fallback to the netstat-based hack otherwise?

@super3
Copy link
Contributor Author

super3 commented Jan 19, 2016

No problem. Part of our own PEP8 cleanup, so I figured best to contribute
back.

I'll tackle them tomorrow morning. Tested locally and on Travis-CI. Should
be zero functional changes.
On Jan 19, 2016 6:35 PM, "yimingliu" notifications@github.com wrote:

Thanks! I haven't had a chance to test it, but I've been meaning to
PEP8-fy this module and just never had the time.

Could I get you to do two minor things?

In the new NatPMP class, the proto variable is hard-coded inline to 1
and 2. Could use the NATPMP_PROTOCOL_TCP and NATPMP_PROTOCOL_UDP
constants already in the module to make it more readable.

I wish something netifaces existed back when I wrote this first, and
would definitely have used it, since gateway detection was the most fragile
part of the code. However, now I have legacy code deployed based on
NATPMP.py, that doesn't rely on installing a third-party dependency. Can
you do an ImportError exception check in get_gateway_addr to see
netifaces can be imported, and fallback to the netstat-based hack otherwise?


Reply to this email directly or view it on GitHub
#8 (comment).

- Removed magic numbers
- Add back netstat-hack
- Removed unused vars
@super3
Copy link
Contributor Author

super3 commented Jan 20, 2016

@yimingliu Believe I implemented what was requested.

yimingliu added a commit that referenced this pull request Jan 20, 2016
Conform to PEP8.  Convenience class NatPMP.  New gateway detection using netifaces.  Thanks to @super3!
@yimingliu yimingliu merged commit 93773fa into yimingliu:master Jan 20, 2016
@super3
Copy link
Contributor Author

super3 commented Jan 20, 2016

Note credit goes to @robertsdotpm for the get_gateway_addr() with netifaces. I just did the PEP8 stuff.

@super3 super3 deleted the patch-1 branch January 20, 2016 18:34
jaraco added a commit to jaraco/nat-pmp that referenced this pull request Feb 10, 2021
Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
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.

2 participants