dmendel / bindata

BinData - Reading and Writing Binary Data in Ruby
BSD 2-Clause "Simplified" License
577 stars 55 forks source link

class of choice is BinData::Choice not the class of the current selection #6

Closed acds closed 11 years ago

acds commented 11 years ago

If a record is defined such as

class MyRecord < ACEARecord

  CHOICES = {
    1 =>  RecordA,
    12 => RecordB,
    20 => RecordC,        
    # ... etc.
  }

  uint8  :message_type, :initial_value => 12
  choice :message_body, :selection => :message_type,
              :choices => CHOICES
end

If one instantiates an instance of MyRecord and queries the type of message body:

a = MyRecord.new

a.message_body.class
=> BinData::Choice

What would be preferable (in the case of the initial_value) is:

=> RecordB

Looking at the code the instantiate code seems to perform a new but there does not seem to be a method for accessing it ?

The end goal would be to have behavior based of the class of the selection such that the following code would work:

case a.message_body
  when RecordA
    # do something based of the context of RecordA
  when RecordB
    # do something based of the context of RecordB
  when RecordC
    # do something based of the context of RecordC

  # ... etc
end
dmendel commented 11 years ago

Yes Choice is a delegate class. BinData (like Ruby) uses duck typing extensively.

If you want to base behaviour on the class then

case CHOICES[a.message_body.selection]
when RecordA
  ...

This looks ugly because it is an ugly way to do it.

Consider creating a def do_something method on Record[ABC] and call it with

a.message_body.do_something
acds commented 11 years ago

Due to roles and responsibilities, coupling and cohesion I want/need to keep functionality separate. The BinData classes have a role to consume a binary stream, and parse out the data so it can be consumed elsewhere.

It seems that Choice on it's, own per the documentation in the class does the right thing (I'll test) and that the Record DSL seems to not expose the same encapsulated behavior?

dmendel commented 11 years ago

I understand your use-case, but I'm not convinced that BinData::Choice#class should return the class of the current selection. Firstly it breaks backwards compatibility, and secondly it's inconsistent with the way delegation works in the rest of BinData.

I'll think it over.

Meanwhile, I suggest you do something like the following:

class BinData::Record < BinData::Struct
  def record_type
    self.class.name
  end
end
case a.message_body.record_type
when "RecordA"
...
acds commented 11 years ago

Thanks for you engagement and responses. I agree that your approach "works" but does not seem tidy and as elegant as my proposal above.

If you create a nested structure of BinData::Record or BinData::Primitive derived classes and request the class of that member it return the true implementation class (not its name - the class). Is this what you mean by the way delegation works ?

I cant imagine for any usage domain where returning BinData::Choice is valuable, and therefore the risk of breaking backwards compatibility is likely low ?

I'm reluctant to fork and work out the appropriate "fix" along the lines of this in BinData::Choice

module BinData

  ...

  class Choice < BinData::Base

    ...

    def class
        current_choice.class
    end

    ...
  end
end

or do I need to change class << self ? I'm a meta programming novice.

Thnaks!

dmendel commented 11 years ago

I've considered the ramifications of your request.

I won't make the change because:

Your above "fix" won't work. You're welcome to try but you'll find it causes breakages elsewhere.

acds commented 11 years ago

Hi @dmendel , thanks for looking into this. As you say, I'm stuck with the workarounds.

In reference to your reasoning, I appreciate, having dug deeper, that the change is much more complex than I'd first perceived, and way beyond my skill set to change. I hope that it is something that might consider re-reviewing in the future.

I'd opinion though in terms of your immutability argument, given this is modeling a choice, by default the structure and therefor the class of the choice can change, and for consistency should match the class of the elements of the structure specification being modeled ? As things stand the implementation forces the choice structure to be anonymous. Perhaps the challenge of the meta programing has forced the current model as it is ? I know this is challenging stuff.

By implementation the structure is reusable and changes it's from based of the current selection between all choices, and can adapt the structure in other ways given other available semantics based of the underlying specification and data read (like optional elements). Do not these aspects also impact immutability, if, between uses, elements just disappear (not just change class name) ?

I hope at some point I'll have the skills to be able to add value, and currently need to focus on delivery not elegance.

Anyways. Thanks again for putting this library/gem together, as I'd be lost with out it, limitations and all.

dmendel commented 11 years ago

Your feature request is beyond the scope of what Choice is designed to do. Read http://bindata.rubyforge.org/manual.html#choices for a definition of Choice.

What you want is a different type of data structure. If you submit an implementation for your proposed type I'll consider adding it to core, or the wiki.

acds commented 11 years ago

I guess the design parameters of BinData are largely centered around coupling the processing of the data into the parsing of the data from the source, and hence my use case does not fit, given the do_something approach.

No I think I want to use the Choice as described; just preferably for the selected choice to be the type/class of the choice as defined in the declarative data structure defined by the BinData DSL, not BinData::Choice, duck typing (as you put it) the type/class in the definition.

Chances are were are "lost in translation", and I thank you for your time and I'll try and move forwards with workarounds....

As a point of note, my initial work around approach as indicated above does not seem to work (as a reference to others that read this challenged by the same issue):

case CHOICES[a.message_body.selection]
when RecordA

It seem that given case/when under the covers uses === equivalence, one actually needs to create an instance on the LHS (case) for the evaluation to render true, resulting in:

case CHOICES[a.message_body.selection].new
when RecordA

Which is really ugly, as it creates a new instance every time the case is evaluated, just to complete a comparison (caching these just makes the whole thing even more obtuse, and in a concurrent environment more difficult to make thread safe) ! I hope you now see why having the real class here is valuable.

Again, thank you for taking the time to followup and respond.

dmendel commented 11 years ago

Yet another way to achieve what you want.

Monkey patch BinData::Choice

class BinData::Choice
  def selected_object
    current_choice
  end
end

case a.message_body.selected_object
when RecordA
...

This won't be added to core due to the potential for aliasing.

acds commented 11 years ago

Hi @dmendel Thanks! This seems to work. I'm not understanding the aliasing issues (still somewhat of a ruby novice). It gets me closer to where I currently need to be, and expect your wisdom will be more clear in time...

Thanks again!