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

[Ruby 3.0 support] Add support for Array subclass methods to return Array #2510

Merged

Conversation

Strech
Copy link

@Strech Strech commented Oct 19, 2021

This PR will add adjust Array that its subclasses on below methods return Array (from the Ruby 3.0 support #2453)

  • Array#drop
  • Array#drop_while
  • Array#flatten
  • Array#slice!
  • Array#slice / Array#[]
  • Array#take
  • Array#take_while
  • Array#uniq
  • Array#*

I find out that for Array.slice I would need to adjust Java code. I will dig a bit to get the context around, but I probably would need some help (or an example)

src/main/ruby/truffleruby/core/array.rb Outdated Show resolved Hide resolved
src/main/ruby/truffleruby/core/array.rb Show resolved Hide resolved
src/main/ruby/truffleruby/core/array.rb Outdated Show resolved Hide resolved
src/main/ruby/truffleruby/core/array.rb Outdated Show resolved Hide resolved
src/main/ruby/truffleruby/core/array.rb Show resolved Hide resolved
@eregon
Copy link
Member

eregon commented Oct 20, 2021

I will dig a bit to get the context around, but I probably would need some help (or an example)

You're welcome to ask any question on Slack :)

@eregon
Copy link
Member

eregon commented Oct 20, 2021

Sorry about the incorrect classification as [easy, pure ruby], it sounds like this change needs more Java changes than I anticipated. However, I think these Java changes should be relatively easy.

@Strech Strech marked this pull request as ready for review October 22, 2021 21:57
@Strech
Copy link
Author

Strech commented Oct 22, 2021

@eregon Thanks for the review and tips, definitely helps! I've resolved all the notes, but I have some doubts about one where I should return the comment in place 🤔

@Strech Strech requested a review from eregon October 23, 2021 10:21
@Strech Strech force-pushed the add-array-methods-return-array-support branch from 3ff6672 to 15d2f59 Compare October 23, 2021 10:25
@Strech
Copy link
Author

Strech commented Oct 26, 2021

👋🏼 Hola @eregon may I ask for one more review? I've applied all the tips you gave me and seems all check passed (but before I squash everything and pushed)

@eregon
Copy link
Member

eregon commented Oct 26, 2021

Could you rebase to latest master? When you do, just drop the changes to spec/truffleruby.mspec as on master RUBY_VERSION is already 3.0.2 and so there is no need to change :next.

@eregon
Copy link
Member

eregon commented Oct 26, 2021

Ah and instead of :next you'll want to run jt untag spec/ruby/core/array to remove tags for now-passing specs.

@eregon eregon self-assigned this Oct 26, 2021
@eregon eregon added this to the 22.0.0 milestone Oct 26, 2021
@Strech Strech force-pushed the add-array-methods-return-array-support branch 2 times, most recently from 4ba1774 to 01859b6 Compare October 26, 2021 14:32
@Strech Strech force-pushed the add-array-methods-return-array-support branch from 1c51824 to 2c7cc10 Compare October 26, 2021 18:24
@Strech
Copy link
Author

Strech commented Oct 26, 2021

Thank you one more time @eregon. All fixes were applied

@Strech Strech requested a review from eregon October 27, 2021 10:23
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you!

@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Oct 27, 2021
@eregon eregon mentioned this pull request Oct 27, 2021
82 tasks
graalvmbot pushed a commit that referenced this pull request Oct 27, 2021
@graalvmbot graalvmbot merged commit 2c7cc10 into oracle:master Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. oca-signed shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants