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

generated API should be generic, eg: stream.readExampleMessage => stream.toProto(ExampleMessage) #10

Open
timotheecour opened this issue Apr 2, 2018 · 4 comments

Comments

@timotheecour
Copy link

timotheecour commented Apr 2, 2018

stream.readExampleMessage (as in https://github.com/PMunch/protobuf-nim/blob/master/README.rst) is not generic (eg can't be used in generic code), and also pollutes namespace with all the generated symbols.
Instead of stream.readExampleMessage how about:
option 1: stream.toProto(ExampleMessage) (preferred)
option 2: stream.toProto[ExampleMessage]
see https://github.com/nim-lang/Nim/issues/7430#issuecomment-377412261 for related discussion between option 1 vs option 2 (option 1 should be preferred)

NOTE: not sure if API generates other non-generic symbols such as readExampleMessage but same comment would apply

@PMunch
Copy link
Owner

PMunch commented Apr 2, 2018

Well, that API is intended to be consistent with the streams module API. I agree that having to (from the linked issue) would be a good idea, and to extend that I would love to see a generic read in the streams module as well. What I can do is of course add a generic read procedure. It won't mitigate the name pollution though.

@timotheecour
Copy link
Author

in the meantime #12 would mitigate this

@PMunch
Copy link
Owner

PMunch commented Apr 3, 2018

I've made a PR to the streams module adding the generic read procedure. If that get's accepted I will add this in for sure.

@timotheecour
Copy link
Author

you gave wrong link in #10 (comment) , shd be nim-lang/Nim#7481

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