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

Add peer_ip and peer_port fields to Conn #26

Closed

Conversation

chrismccord
Copy link
Contributor

This PR adds peer_ip, peer_port fields to the Conn. I wasn't sure how to get at the ranch/cowboy request to setup the peer_port, so the test for peer_port currently just asserts that it's a present and valid integer.

@josevalim
Copy link
Member

Thanks! What if we just call it peer returning a tuple? And for tests, we always run them using the same port, so you can assert whatever you got there.

@chrismccord
Copy link
Contributor Author

I split them since my own prior use-case almost always involve grabbing the peer's ip only, but a tuple would be just fine. I'll make the change.

As far as testing, the peer_port actually randomly changes across test runs so I can't reliably check its value.

@josevalim
Copy link
Member

Awesome, thanks!

@chrismccord
Copy link
Contributor Author

The requested changes have been made. Let me know if you would like to see any changes.

@josevalim
Copy link
Member

Thanks. I am wondering if we should leave the ip as a tuple or a binary. Do you have any reasoning for choosing one over the other? At first I prefer the tuple because it is already the parsed data but I am not sure.

@ericmj
Copy link
Member

ericmj commented Mar 23, 2014

@chrismccord Don't forget to add the peer field to record_type.

@josevalim What is the reasoning for not using the parsed tuple? I think we should keep the tuple, it is easier to convert a tuple to binary than the other way around.

@chrismccord
Copy link
Contributor Author

I like the ip as a tuple since we don't have to do any extra work, like you said. Usability wise, the ways I would use the ip would likely be in string form most of the time, but that's only a single function call to obtain. Perhaps we use a tuple for the ip and provide a helper function to convert the peer to a string ip? This would provide easier conversion access instead of users having to dig into inet and convert to a string. Thoughts?

@josevalim
Copy link
Member

This would provide easier conversion access instead of users having to dig into inet and convert to a string.

Sounds like a good plan. How should it be called and where should it be?

Another option is to make it return a Plug.Conn.Peer struct and implement String.Chars for it.

@chrismccord
Copy link
Contributor Author

I like the idea of the struct/protocol. Would the String.Chars for a Peer just convert the IP to string?

@josevalim
Copy link
Member

@chrismccord I would make it return "IP:PORT" but I would also be ok with it returning "IP" only (leaving the port out). However, for now we would need to implement it as a record.

@chrismccord
Copy link
Contributor Author

I'll get started on this with records and see how it looks

@josevalim
Copy link
Member

@chrismccord is there still any interest here? :)

@josevalim
Copy link
Member

Closing this, we can always add it back when there is a need. :)

This pull request was closed.
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.

3 participants