Skip to content

Conversation

PMunch
Copy link
Collaborator

@PMunch PMunch commented Mar 30, 2018

No description provided.

@dom96
Copy link
Contributor

dom96 commented Mar 30, 2018

No dashes allowed in package names. Also please add new packages at the bottom.

@PMunch
Copy link
Collaborator Author

PMunch commented Mar 30, 2018

Oh woops, didn't mean to put the dash in there, was just doing a multi-replace for github URL, website, and name.

@dom96
Copy link
Contributor

dom96 commented Apr 1, 2018

Some remarks:

For combparser you should really use the options module instead of creating your own Maybe type. Although I see your Maybe type is in fact an Either type. Consider renaming :)

I wonder what we should do about @oswjk's package? https://github.com/oswjk/protobuf-nim. It seems you submitted this PR first so I guess you win this name, but it still sucks a bit for @oswjk, any thoughts?

@PMunch
Copy link
Collaborator Author

PMunch commented Apr 1, 2018

Yeah, combparser should really be rewritten from scratch. It was all based this two year old snippet written in a very functional style, and not very Nimish (Nim-esque? Nim-like? Nim-tastic? Nim-tacular? What do we use to describe things that are "Nim", similar to "Pythonic"). The original Maybe type should be renamed something else entirely, and the entire structure should be given a second (or third) thought. I just needed to publish it to get protobuf published.

I was wondering the same thing about @oswjks package, actually a bit relieved to find he didn't beat me to it so I didn't have to think up a better name :P I'd be happy to have none of them use "protobuf" though, and let them both have some descriptive suffix. But it's hard coming up with some short suffix that accurately describes the difference between them. A second option is of course to merge them into a super-package. If the protoc plugin could spit out an object initialisation for my ProtoNode object structure you would be able to write a similar function to what he's doing and generate the same code as I'm doing now.

@oskaritimperi
Copy link
Contributor

oskaritimperi commented Apr 1, 2018

@PMunch I'm okay with you taking the name :-)

I have taken a bit new direction with my plugin/library, basically just spitting ready Nim code out which doesn't need any compile-time evaluation to be done at it. That stuff hasn't been pushed yet though. I was thinking that why have a protoc plugin which can generate code and then have the Nim compiler again generate code, when you can just generate the code once. This is now the difference between my library and yours.

@dom96
Copy link
Contributor

dom96 commented Apr 4, 2018

So, can you fix this PR?

@PMunch
Copy link
Collaborator Author

PMunch commented Apr 4, 2018

Oh sorry, I fixed it ages ago but just forgot to push 😛 I was wondering why it took you so long to merge

@dom96 dom96 merged commit cd85e7c into nim-lang:master Apr 24, 2018
pmetras pushed a commit to pmetras/packages that referenced this pull request Sep 13, 2019
Add protobuf and combparser packages
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