JRuby

[1.9] Struct#members and Struct::members should return an Array of Symbols in 1.9

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: None
  • Fix Version/s: JRuby 1.6.6, JRuby 1.7.0.pre1
  • Component/s: None
  • Labels:
    None
  • Patch Submitted:
    Yes
  • Number of attachments :
    1

Description

Hello,

This is my first patch to JRuby, so please be teach me where I'm wrong.

I noticed Struct.members return an Array of Strings, even in 1.9.
A patch is attached, it removes the RubySpec tag which was "hiding" this issue.

I'm unsure if the duplication between members18 and members19 is fine, but as it is mostly a loop, it's probably fine.
I used the members18 method in unmarshalFrom(), maybe using the 19 version would be faster since it does not require to convert members to ruby Strings?

Hope it helps,
Benoit Daloze

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Hiro Asari added a comment -

Benoit,

Thanks for the patch. In general, we don't put the suffix '18' for methods in the 1.8 mode. This is making your patch noisier than necessary, I believe.

Show
Hiro Asari added a comment - Benoit, Thanks for the patch. In general, we don't put the suffix '18' for methods in the 1.8 mode. This is making your patch noisier than necessary, I believe.
Hide
Permalink
Benoit Daloze added a comment -

Here is the updated patch, which do not rename members to members18.
Indeed, the patch looks cleaner, thanks for your advice.

I also used members19() in unmarshalFrom() since it avoids converting the members to RubyStrings.

Show
Benoit Daloze added a comment - Here is the updated patch, which do not rename members to members18. Indeed, the patch looks cleaner, thanks for your advice. I also used members19() in unmarshalFrom() since it avoids converting the members to RubyStrings.
Hide
Permalink
Hiro Asari added a comment -

Benoit,

Thank you for updating the patch. I hate to push this back again, but I have to say this: the patch should contain only one idea (I am sometimes guilty of mixing things up, too.); so the 'unmarshal' bit should be split off this patch to keep the issues separate.

We are almost there, I think.

Show
Hiro Asari added a comment - Benoit, Thank you for updating the patch. I hate to push this back again, but I have to say this: the patch should contain only one idea (I am sometimes guilty of mixing things up, too.); so the 'unmarshal' bit should be split off this patch to keep the issues separate. We are almost there, I think.
Hide
Permalink
Benoit Daloze added a comment -

Sure, I understand. Here it is then.

It might be worth to mention in the wiki that you really like one single concern per patch though.

Show
Benoit Daloze added a comment - Sure, I understand. Here it is then. It might be worth to mention in the wiki that you really like one single concern per patch though.
Hide
Permalink
Hiro Asari added a comment -

Thanks, Benoit.

I merged the patch to the master (64375dc) and the 1.6 branch (6348138).

Show
Hiro Asari added a comment - Thanks, Benoit. I merged the patch to the master (64375dc) and the 1.6 branch (6348138).

People

  • Assignee:
    Hiro Asari
    Reporter:
    Benoit Daloze
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: