Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add keyword_init option to Struct.new #5061

Merged
merged 1 commit into from Mar 21, 2018

Conversation

@nomadium
Copy link
Contributor

@nomadium nomadium commented Feb 23, 2018

Hi folks,

This is another feature targeting Ruby 2.5 [1]: Add keyword_init option to Struct.new (feature #11925).

Thanks for your review and feedback.

  1. #4876
@nomadium
Copy link
Contributor Author

@nomadium nomadium commented Feb 23, 2018

I gave a shot to the implementation of this new feature and although all existing tests (and the new ones exercising this change) in test/mri/ruby/test_struct.rb are passing, I'm probably missing something here.

I decided to test if inheritance work as expected and I wrote the following test to verify but I found out some assertions are failing:

  def test_struct_new_with_keyword_init_and_inheritance
    klass = @Struct.new(:a, keyword_init: true)
    klass2 = Class.new(klass)
    assert_raise(ArgumentError) { klass2.new(1) } # broken, nothing is raised: => #<struct a=1>
    assert_nothing_raised { klass2.new(a: 5) }    # nothing is raised but is wrong as well: => #<struct a={:a=>5}>
    o = klass2.new(a: 5)                          # wrong => #<struct a={:a=>5}>
    assert_equal(5, o.a)                          # broken: o.a => {:a=>5}
  end

Where should I look to get this working?

BTW, do you see worthwhile to contribute that extra test to MRI or ruby/spec?

@headius
Copy link
Member

@headius headius commented Feb 23, 2018

Thank you for the PR!

Regarding your test:

  • Contribute it to ruby/spec, ideally. It will filter back to JRuby whenever we update. ruby/spec is structured better and should always be the first place you look to contribute tests.
  • If you want to try to fix it, look at RubyStruct.createStructClass and newInstance and compare with CRuby's logic. It's possible they changed the class structure surrounding Struct and we missed it.
  • You can also defer and file a separate bug for the failing assertions, so we can get the PR in. We'll collab on fixing it in-place then.
@headius headius added this to the JRuby 9.2.0.0 milestone Feb 23, 2018
@headius headius added the ruby 2.5 label Feb 23, 2018
@nomadium
Copy link
Contributor Author

@nomadium nomadium commented Feb 25, 2018

@headius Thanks for the pointer, I just saw what's the problem with the inheritance in this case. I'll update the PR soon.

@nomadium nomadium force-pushed the nomadium:keyword-arguments-on-struct-new branch from 10985df to dea2028 Feb 25, 2018
@nomadium
Copy link
Contributor Author

@nomadium nomadium commented Feb 25, 2018

@headius The issue I pointed out in my previous comment is now fixed. I'll try to submit my extra test to ruby/spec later.

throw getRuntime().newArgumentError("unknown keywords: " + unknownKeywords);
}
}

This comment has been minimized.

@nomadium

nomadium Mar 9, 2018
Author Contributor

I realised I was reimplementing functionality already available in JRuby with these checks. I'll update this PR soon to address that.

@nomadium nomadium force-pushed the nomadium:keyword-arguments-on-struct-new branch from dea2028 to bfa648a Mar 10, 2018
For more information, please see feature #11925.
@nomadium nomadium force-pushed the nomadium:keyword-arguments-on-struct-new branch from bfa648a to 6746380 Mar 11, 2018
@nomadium nomadium mentioned this pull request Mar 20, 2018
64 of 75 tasks complete
@headius headius merged commit 55fa773 into jruby:ruby-2.5 Mar 21, 2018
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@nomadium nomadium deleted the nomadium:keyword-arguments-on-struct-new branch Mar 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.