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

[3.2 compatibility] Make an empty hash shift return nil #2980

Conversation

itarato
Copy link
Collaborator

@itarato itarato commented Apr 4, 2023

#3039

Ruby 3.2 empty Hash, when #shift is applied returns a nil. This PR makes this update for truffleruby.

Hash#shift now always returns nil if the hash is empty, instead of returning the default value or calling the default proc. [Bug #16908]

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Apr 4, 2023
@itarato itarato closed this Apr 4, 2023
@itarato itarato reopened this Apr 4, 2023
@andrykonchin
Copy link
Member

Thank you. Changes look good for me.

Regarding the failed specs - will look at the issue soon.

@andrykonchin
Copy link
Member

I am not able to push my changes into this remote branch because of the following error.

ERROR: Permission to Shopify/truffleruby.git denied to andrykonchin.

So this PR will be marked as merged when an internal PR is merged.

The following test cases in test_array.rb fail now:
- test_shift2
- test_shift_none
@andrykonchin andrykonchin added the in-ci The PR is being tested in CI. Do not push new commits. label Apr 6, 2023
@nirvdrum
Copy link
Collaborator

nirvdrum commented Apr 7, 2023

@andrykonchin I don't think we need to tag any failing specs. Those specs will fail in master because master targets Ruby 3.1. We should have a 3.2-upgrade branch that either changes the version number to 3.2.2 or runs all specs with :next. At least, that's my understanding of the process. We don't support more than one Ruby version at a time, so this PR can't be merged into master yet.

@andrykonchin
Copy link
Member

andrykonchin commented Apr 7, 2023

I've canceled the merging workflow for this PR. Will discuss it with @eregon next week.

AFAIK our current approach is to merge only not harmful changes introduced in the next (not supported yet) Ruby version. Both new features and bug fixes I would consider as not harmful TBH. And this PR looks like just fixing a bug.

@andrykonchin andrykonchin removed the in-ci The PR is being tested in CI. Do not push new commits. label Apr 7, 2023
@eregon
Copy link
Member

eregon commented Apr 11, 2023

I discussed it with Andrii. CRuby chose to not backport this, so it's in 3.2 but not in 3.1.
The potential of incompatibility seems pretty low (who would rely on this? Any gem testing against 3.1 & 3.2 has to handle this change, and it's easy to feature-detect this change), so I think it's OK to merge this to master.
In case of trouble we can always revert it and reapply it later.

We should have a 3.2-upgrade branch that either changes the version number to 3.2.2 or runs all specs with :next.

@nirvdrum We never had version-upgrade branches, because it's unfeasible in terms of conflicts (especially for the import files from MRI part). Specs listed in :next are already run in GH and internal CI.

@graalvmbot graalvmbot merged commit e845eda into oracle:master Apr 11, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement. shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants