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

Match out of range ArgumentError message with MRI #1774

Merged
merged 3 commits into from
Oct 30, 2019

Conversation

rafaelfranca
Copy link
Contributor

Active Support is asserting on this message, so I thought we could get it to match.

Shopify#1

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.

Agreed in general we should match MRI exception messages for compatibility, unless there is a good reason no to (pretty rare).

src/main/java/org/truffleruby/core/string/CoreStrings.java Outdated Show resolved Hide resolved
@eregon eregon self-assigned this Oct 28, 2019
@eregon eregon added this to the 20.0.0 milestone Oct 28, 2019
@rafaelfranca rafaelfranca force-pushed the rm-fix-out-of-range-error branch from b07ff12 to 96b24bc Compare October 28, 2019 18:48
@rafaelfranca
Copy link
Contributor Author

Added the CHANGELOG entry

@eregon
Copy link
Member

eregon commented Oct 29, 2019

@rafaelfranca Could you rebase this PR on top of current master (which has the CHANGELOG fixes)?

@rafaelfranca rafaelfranca force-pushed the rm-fix-out-of-range-error branch from 96b24bc to f38d3ca Compare October 29, 2019 13:46
@rafaelfranca
Copy link
Contributor Author

Done

@rafaelfranca rafaelfranca force-pushed the rm-fix-out-of-range-error branch from f38d3ca to 176d2c3 Compare October 29, 2019 13:52
@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Oct 29, 2019
graalvmbot pushed a commit that referenced this pull request Oct 30, 2019
@graalvmbot graalvmbot merged commit 176d2c3 into oracle:master Oct 30, 2019
@eregon
Copy link
Member

eregon commented Oct 30, 2019

Merged, thanks.
I found some weird behavior with month=13 on MRI and added a spec for that case too: 564ebe4

@rafaelfranca
Copy link
Contributor Author

rafaelfranca commented Oct 30, 2019

Yeah.... I found that too but chose to not spec it since our behavior would be different. The reason for that behavior is that MRI uses 4 bits to store the value for month. If the value is inside the 0-15 range it can be stored in the struct and later checked against the range of valid months. If the value is outside that range it will raise an error earlier.

@eregon
Copy link
Member

eregon commented Oct 30, 2019

Interesting, thanks for the explanation and links.
I allowed both messages in the spec as indeed I don't think the difference is really intended.

@chrisseaton chrisseaton deleted the rm-fix-out-of-range-error branch December 3, 2019 20:48
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