-
Notifications
You must be signed in to change notification settings - Fork 111
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
Enable String#to_i spec #2224
Enable String#to_i spec #2224
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good so far and I'm excited to turn the specs on. I think there are some spots where this code still drifts from MRI.
For the cases that are not covered by the specs, can you add some integrations tests to:
artichoke/artichoke-backend/src/extn/core/string/string_test.rb
Lines 5 to 16 in 255dd13
def spec | |
string_match_operator | |
string_element_reference_regexp | |
string_byteslice | |
string_scan | |
string_unary_minus | |
string_reverse | |
string_tr | |
string_end_with | |
true | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great! thank you for the logic fixes, the new specs, and the new integration tests
Relates to #1912
Currently
String#to_i
isn't in the enforced specs, and there deviations from MRI because of this.This PR makes
to_i
spec compliant, asides from the cases where aBignum
is needed which now throw aNotImplemented
error instead.While doing this work I did attempt to use the
scolapasta-int-parse
implementation, however asto_i
parses as much as possible before failure and scolapasta either passes or returns an error I would have had to make more changes that I felt comfortable with in that library to make the specs pass. This has lead to a duplication of logic, but there is already an issue to fix this (#2075)