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

Replace array flattening with manual handling in java_import #2334

Merged
merged 1 commit into from Jan 12, 2015

Conversation

lumeet
Copy link
Contributor

@lumeet lumeet commented Dec 18, 2014

Fixed #2324 by getting rid of flatten! as hinted by @headius. I thought about writing a test for the method, too, as I couldn't find any. Too bad, I couldn't find a way to run a single test in tests/jruby either. Is that the right place for such tests? Help appreciated.

@headius
Copy link
Member

@headius headius commented Jan 12, 2015

You could put the tests in test/jruby (they should be runnable directly, with only a few counterexamples) or in spec/regression. Former would probably be best since usually the latter is for Ruby behaviors (rather than JRuby behaviors) that aren't tested well in other suites.

FWIW, this will get exercised heavily by other Java integration tests too.

@headius
Copy link
Member

@headius headius commented Jan 12, 2015

Two comments on the commit:

  • If there's any array-like objects passed to java_import, this will cause them to no longer coerce. It seems highly unlikely that someone would have that case in real code, though, and I don't think we've ever officially supported such wildly-structured import lists.
  • Formatting is a little weird in the patch. I can fix that on the way in.

We'll only make this change on master, since it does have a potentially-visible behavior change.

@headius headius merged commit 3dc06a8 into jruby:master Jan 12, 2015
1 check failed
@headius headius added this to the JRuby 9.0.0.0-pre1 milestone Jan 12, 2015
@headius headius self-assigned this Jan 12, 2015
@lumeet lumeet deleted the 2324 branch Jan 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants