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

Make all things Optionable and fix requires #316

Closed embark closed 8 years ago

embark commented 8 years ago

In prep for adding support for custom field, enum, message, file, service, and method options

CC @nerdrew @film42 @liveh2o

Re: requires

get_option needs Google::Protobuf::_Options to work (Optionable takes a Google::Protobuf::_Options class as an argument), so descriptor.pb needs to get required before using get_option.

BUT, to parse descriptor.pb, Protobuf::{Enum,Message,etc} needs to exist. So, require protobuf/message, then require descriptor.pb

get_option! -> return explicitly set options only

embark commented 8 years ago

Tests will fail until #315 is merged

embark commented 8 years ago

@zanker @zachmargolis care to review?

zanker commented 8 years ago

At least code wise it looks sane, I don't have a lot of context on the protobuf specifics though.

nerdrew commented 8 years ago

LG. KEEP UP THE GOOD WORK

embark commented 8 years ago

Ping @film42 @liveh2o . Tests expected to fail until my other PR is merged, sorry about that

film42 commented 8 years ago

Just merged #315

film42 commented 8 years ago

Let's get this branch cleaned up and I think it should be good to go.

embark commented 8 years ago

What have I done to rubocop :/

embark commented 8 years ago

@film42 sorry for the delay, fixed rubocop errors by disabling the broken cop

embark commented 8 years ago

@film42 bump :)

zachmargolis commented 8 years ago

Hey so I know this is late in the life of this PR but I think we can clean up he markup by a lot, and not have to include that inject line if we use the Class#inherited hook

So instead of having

class Message < Protobuf::Message
  Protobuf::Optionable.inject(self)
end

We can define #inherited on the Message class:

class Protobuf::Message
  def self.inherited(subklass)
    Protobuf::Optionable.inject(subklass)
  end
end

Which has the same net effect and cleans up the result. WDYT?

embark commented 8 years ago

@zachmargolis If I understand what you're saying, I don't think that's going to work the way you want. We're actually injecting FileOptions on every inner module that we get from the package name only (none of which extend anything). For example:

module Google
  module Protobuf
    ::Protobuf::Optionable.inject(self) { ::Google::Protobuf::FileOptions }

The Protobuf::Message inject is already inherited by default with this code:

module Protobuf
  class Message
    ::Protobuf::Optionable.inject(self) { ::Google::Protobuf::MessageOptions }
zachmargolis commented 8 years ago

@embark oh you are right I completely misunderstood, thx for explaining

embark commented 8 years ago

@film42 @nerdrew changed the require logic structure, makes things ultimately more simple I think. Let me know what you think

embark commented 8 years ago

ping @film42 @liveh2o for review :)

film42 commented 8 years ago

Thanks for the ping @embark ! :shipit: 🎉

Also, thanks for your patience :)

I probably won't be able to cut a pre until tonight, but I'll set a reminder so I don't forget!

embark commented 8 years ago

Thanks! Please give it another look before you merge

film42 commented 8 years ago

Released as 3.7.0.pre1!