[jira] Commented: (THRIFT-697) Union support in Ruby

["Kevin Clark (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/THRIFT-697?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12834099#action_12834099 ] 

Kevin Clark commented on THRIFT-697:
------------------------------------

Generally looks fine, but could use some cleanup. Some of this is nitpicky, so take it with a grain of salt. My notes follow.

struct.c
There's a dangling TODO (to raise an exception) in rb_thrift_union_read and another in _write. For read, we should throw and remove the TODO. For write, it looks like it comes down to a .to_sym call in the Ruby. Maybe it'd be worth benchmarking?


union.rb
If you think the current version is inelegant, I think you can rewrite Thrift::Union#initialize as:
def initialize(name = nil, value = nil)
  if name && value.nil?
    raise Exception ..
  end
  
  if name
    Thrift.check_type ...
    @field_id = name_to_id(name.to_s)
  end

  @setfield = name
  @value = value
end

It avoids an extra call to name_to_id (and the .to_s) and reads a little cleaner.

You may want to benchmark if there's a difference in performance for uses of define_method vs class eval. I thought I remember there being a significant difference, but that might not be the case anymore.

Why the handwritten getter/setter (and then a protected version of the exact same thing)? Feels very un-ruby

Is the 'set' required used anywhere?


struct_union.rb
I know it was moved from before, but is there a reason each_field needs to exist? Seems like a needless sort/copy/method call, but maybe there's a reason we were doing this that I've forgotten? If it does need to exist, is there a reason name_to_ids doesn't use it?

union_spec.rb
Is there a reason you're testing that .new does't return nil? I'm not sure how to make that fail.


t_rb_generator.cc
Looks like there might be some spacing issues with the if in generate_struct (if tstruct->is_union)

In generate_union_accessors, I'd probably move the declaration of m_iter into the for, or atleast into the if clause, but it's really nitpicky and I don't have an issue with it staying that way.

If we're going to do a copy (from f_iter to field) in generate_rb_union_validatior, might as well declare field as const.

> Union support in Ruby
> ---------------------
>
>                 Key: THRIFT-697
>                 URL: https://issues.apache.org/jira/browse/THRIFT-697
>             Project: Thrift
>          Issue Type: New Feature
>          Components: Compiler (Ruby), Library (Ruby)
>    Affects Versions: 0.3
>            Reporter: Bryan Duxbury
>            Assignee: Bryan Duxbury
>             Fix For: 0.3
>
>         Attachments: thrift-697.patch
>
>
> We'd like to have direct native support for Union-style structs in Ruby. 

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.