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

Enhancement RFC: Used namedtuple for remote #12

Open
jlitzinger-kinestral opened this issue May 2, 2016 · 3 comments
Open

Enhancement RFC: Used namedtuple for remote #12

jlitzinger-kinestral opened this issue May 2, 2016 · 3 comments

Comments

@jlitzinger-kinestral
Copy link
Contributor

Apologies, I'm hijacking the Issues tab to discuss a feature proposal...not sure if that's cool or not, but it's what they offer. Anyway, as I use txThings (which I like, nicely done), I noticed that storing the remote in a namedtuple with host/port fields might read a little easier. Thoughts?

E.g. Assuming a remote of ::1:5683 and a message object named request
IS:

request.remote = ('::1', 5683)
host = request.remote[0]
port = request.remote[1]

Proposed:
A named tuple such that:

Remote = collections.namedtuple('Remote', ['host', 'port'])
request.remote = Remote('::1', 5683)
host = request.remote.host
port = request.remote.port

Additionally, the above index syntax should still work, maintaining backward compatibility for existing clients.

Certainly not required, but conceptually more direct (to me) than [0], [1]. If you are not opposed, I'll make the change and send a PR. In that case, let me know whether you'd like existing uses modified.

@mwasilak
Copy link
Owner

mwasilak commented May 3, 2016

Seems sensible. Please note that I changed remote tuple to store host address as ipaddress.ip_address and not as simple string (Issue #10).

@jlitzinger-kinestral
Copy link
Contributor Author

Hmm, I knew that, and forgot prior to writing this suggestion, sorry. Let me review that change and re-evaluate my suggestion above. The ipaddress module may have already achieved the goal I suggested above.

@jlitzinger-kinestral
Copy link
Contributor Author

jlitzinger-kinestral commented May 5, 2016

Bit of a tangential question, was there a reason you selected py2-ipaddress over ipaddress (https://github.com/phihag/ipaddress)?

I have no experience or preference for one or the other, however, Twisted (optionally) requires cryptography, the latest version of which requires ipaddress. Since you target Twisted specifically I wondered if there was a deliberate reason you chose py2-ipaddress.

I suspect they are similar, but, there is a difference as noted on the py2-ipaddress page. Specifically, ipaddress requires unicode objects vs str objects for constructing ipaddresses, whereas py2-ipaddress accepts both. Additionally, py2-ipaddress uses bytearrays for the binary representation, whereas ipaddress uses strs.

I only bring this up because installing py2-ipaddress, at least for me, overwrites the dependency installed by cryptography, which means if one installs txThings in a working Twisted install that uses cryptography, it has silently affected the cryptography module..which is probably not desirable. It may be harmless, I haven't done any verifications at all.

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

No branches or pull requests

2 participants