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

added Integer.try_convert, Ruby 3.1 support #2905

Merged
merged 3 commits into from
Mar 7, 2023

Conversation

moste00
Copy link
Contributor

@moste00 moste00 commented Mar 3, 2023

Fixed an issue listed in #2733, namely the addition of a method try_convert to the class Integer, spec is in spec/ruby/core/integer/try_convert_spec.rb.

@oracle-contributor-agreement
Copy link

Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
The following contributors of this PR have not signed the OCA:

To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application.

When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated.

If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. label Mar 3, 2023
@eregon eregon mentioned this pull request Mar 3, 2023
70 tasks
@eregon
Copy link
Member

eregon commented Mar 3, 2023

Thank you for the PR, I will review after the OCA is through.

@eregon eregon self-requested a review March 3, 2023 18:08
@eregon eregon self-assigned this Mar 3, 2023
@oracle-contributor-agreement oracle-contributor-agreement bot added OCA Verified All contributors have signed the Oracle Contributor Agreement. and removed OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. labels Mar 4, 2023
src/main/ruby/truffleruby/core/integer.rb Outdated Show resolved Hide resolved
src/main/ruby/truffleruby/core/integer.rb Outdated Show resolved Hide resolved
@moste00 moste00 changed the title added Integer.try_convert, required for Ruby3.1 support added Integer.try_convert and modified File.dirname to accept an optional argument, required for Ruby3.1 support Mar 5, 2023
@moste00 moste00 changed the title added Integer.try_convert and modified File.dirname to accept an optional argument, required for Ruby3.1 support added Integer.try_convert, Ruby 3.1 support Mar 6, 2023
@moste00 moste00 force-pushed the IntegerTryConvert branch 2 times, most recently from bcb5269 to e2bb77e Compare March 6, 2023 20:42
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.

Thank you for the PR, I'll do the finishing touches and run the internal CI on it.

It also needs a changelog entry, I'll add it.

src/main/ruby/truffleruby/core/integer.rb Outdated Show resolved Hide resolved
spec/ruby/core/integer/try_convert_spec.rb Outdated Show resolved Hide resolved
@eregon eregon added this to the 23.0.0 Release (April 18, 2023) milestone Mar 7, 2023
@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Mar 7, 2023
@eregon
Copy link
Member

eregon commented Mar 7, 2023

@moste00 Which ruby version did you use to get the error message?
I tried on CRuby 3.1.3 and the message used to, not into:

$ jt -u ruby test spec/ruby/core/integer/try_convert_spec.rb
ruby 3.1.3p185 (2022-11-24 revision 1a6b16756e) [x86_64-linux]
                                                                                             
1)
Integer.try_convert sends #to_int to the argument and raises TypeError if it's not a kind of Integer ERROR
Expected TypeError (can't convert MockObject into Integer (MockObject#to_int gives Object))
but got: TypeError (can't convert MockObject to Integer (MockObject#to_int gives Object))

@eregon
Copy link
Member

eregon commented Mar 7, 2023

The CI tells us in https://github.com/oracle/truffleruby/actions/runs/4347779724/jobs/7611599985
that in is used (Ruby < 3.1 doesn't run those specs of course).
So I'll change to that.

moste00 and others added 3 commits March 7, 2023 16:05
* Fix Truffle::Type.try_convert message to match CRuby.
* Spec the message for all .try_convert TypeErrors.
@moste00
Copy link
Contributor Author

moste00 commented Mar 7, 2023

@moste00 Which ruby version did you use to get the error message? I tried on CRuby 3.1.3 and the message used to, not into:

I was having problems with my distro's apt when I added the specs testing the error message, it seemed to think that ruby 3.0 is the newest version and no amount of updating or upgrading will change its mind. As a temp workaround I used this website https://try.ruby-lang.org/, which, now that I have the idea to run "puts RUBY_VERSION", uses ruby 3.2 under the hood. I apologize for not noticing this earlier, and for not using my local 3.1 as soon as I got it working.

As a curious observation, CRuby changed the error message going from 3.1 to 3.2 without inlcuding this in the release docs or any other docs I'm aware of (or did they ?), the error message also appears to be inconsistent : in ruby 3.1, passing an Object.new that mocks :to_int to return an empty string gives the same standard message as any other non-Integer object (using "to"), but in ruby 3.2, the empty string in particular gets the response "no implicit conversion of Object into Integer", other strings and Object.new get the usual response with "into".

Maybe there is an official subtlety in the type system somewhere that says the empty string in particular should get this particular response, but more likely it seems to me that this is just an implementation detail. It seems also that CRuby doesn't regard error messages as part of the API, this might be an explanation for why nobody wrote a spec to test the message till now. So should we even regard error messages as API-visible changes when CRuby itself doesn't ? Is it plausible that someone is "depending" on the error message being this exact same string somehow ? I don't know, but I think it's low probability.

@eregon
Copy link
Member

eregon commented Mar 7, 2023

https://try.ruby-lang.org/ is using Opal and not CRuby, you can see that with:

p RUBY_DESCRIPTION
p RUBY_VERSION

And Opal is not fully compatible, e.g. they have a different message there.
I will try to find a way to change this, you're not the first one to fall into this trap.
ruby/TryRuby#122

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 Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants