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 provisional implementation for #1081 #1082

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Kentzo
Copy link
Contributor

@Kentzo Kentzo commented Sep 8, 2017

No description provided.

@minrk
Copy link
Member

minrk commented Nov 3, 2017

Sorry for the delay in reviewing this. I've no specific objection, but it doesn't really seem like pyzmq is the right place for it. What's the benefit of this over, say, using the builtin urlparse for parsing URLs, which gives you scheme, port, etc.?

@Kentzo
Copy link
Contributor Author

Kentzo commented Nov 7, 2017

The main benefit is to be able to integrate with various libraries that deal with user input such as CLI toolkits, config parsers, ORM etc. urlparse is too abstract to be useful as it can only represent 2 components of a zeromq endpoint: scheme and netloc. In my development experience I often need to access just a port or just an interface of a TCP endpoint. And there are other, more complex transports.

Although it can be developed as an independent package, having it bundled provide the following benefits:

  • Native support by pyzmq methods, so one can pass an object into bind / connect etc (implementation only needs to wrap arguments with str or repr)
  • Easier to keep it in sync with zeromq API
  • More attention would allow to have one "definitive" implementation

@minrk
Copy link
Member

minrk commented Nov 7, 2017

is too abstract to be useful as it can only represent 2 components of a zeromq endpoint: scheme and netloc

that's not generally true, urlparse extracts host and port as well:

parsed  = urlparse('tcp://127.0.0.1:5555')
parsed.port # 5555
parsed.hostname # '127.0.0.1'

but that doesn't cover more complex non-url transports, such as the multi-endpoint URLs separated by ;.

This is fine by me. What more do you want to do for this PR? Can you add docs and tests?

@Kentzo
Copy link
Contributor Author

Kentzo commented Nov 8, 2017

I would like to see how far it can / should be integrated into pyzmq. I think both connect and bind should be able to accept it, what about last_endpoint?

@minrk
Copy link
Member

minrk commented Nov 8, 2017

I don't think we should change the return type of any of the existing methods, but accepting these objects in bind/connect makes sense.

@Kentzo
Copy link
Contributor Author

Kentzo commented Nov 13, 2017

Perhaps both Address and Endpoint should subclass UserString? Current __iter__ can be replaced with the as_tuple method.

@minrk
Copy link
Member

minrk commented Nov 14, 2017

I think that specifically, no base pyzmq APIs should return anything but basic types. Providing convenient wrappers is okay, but I wouldn't change the types, even if they are subclasses that satisfy isinstance

@Kentzo
Copy link
Contributor Author

Kentzo commented Nov 14, 2017

@minrk Ok, but considering that, would it be better to have them as strings or objects?

@minrk
Copy link
Member

minrk commented Nov 15, 2017

Objects, I think.

@Kentzo
Copy link
Contributor Author

Kentzo commented Nov 15, 2017

UserString would allow to avoid any changes to pyzmq though. Which makes sense, as URL introspection does not make sense there. Not with current API.

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