search

ruby-protobuf / protobuf

A pure ruby implementation of Google's Protocol Buffers
https://github.com/ruby-protobuf
MIT License
463 stars 101 forks source link

Preserve raw RPC name (since #underscore can be lossy) #312

Closed zachmargolis closed 8 years ago

zachmargolis commented 8 years ago

cc @nerdrew @embark

We found some RPC names that were not the same in a roundtrip of name.underscore.camelize so this preserves the raw data

zachmargolis commented 8 years ago

Another option would be to add a compiler env var flag to just use the raw name instead, would simplify a lot of the code

Ex: PB_RAW_RPC_NAMES=true or something

film42 commented 8 years ago

Looks good to me. I'm looking to get some of these bigger PRs merged first, but I'll grab this after.

embark commented 8 years ago

@zachmargolis this code is also probably broken if the rpc names collide when underscored, right? Like in this issue: https://github.com/grpc/grpc/issues/5812

zachmargolis commented 8 years ago

@embark I'll redo this with the env var flag outlined above -- maybe we could make the "raw" name the default behavior in the near future

film42 commented 8 years ago

Added one comment about naming, but otherwise this is :+1: to me. Let's get this fixed/ merged and then release a pre. I'm not sure if it should be 3.7.0.pre0 or 4.0.0.pre0 but it's much too late to debate about that for tonight. If you have an opinion one way or the other, let me know :).

embark commented 8 years ago

Thanks, looks good!

zachmargolis commented 8 years ago

Done! Used PB_USE_RAW_RPC_NAMES

film42 commented 8 years ago

Grrr. Looks like rubo:cop: has a new release out.

zachmargolis commented 8 years ago

How did Rubocop change? Looks like the version was pinned in the gemspec back in 86ef9075a2115904b59612c6152717c475164ea9

film42 commented 8 years ago

Looks like the ruby parser gem had a breaking changed released as a patch verison https://github.com/bbatsov/rubocop/pull/2984#issuecomment-201334316. I'm not able to lock it down and fix it though, so I'm honestly not sure what the issue is. I'll let them sort it out.

:shipit:

film42 commented 8 years ago

Thank you for your PR, btw :)

film42 commented 8 years ago

3.7.0.pre0 has been released.