Skip to content

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).

@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
@chrisseaton chrisseaton added the shopify Pull requests from Shopify label Feb 2, 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 Pull requests from Shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants