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

Fix test with negative numbers #590

Merged
merged 1 commit into from Nov 16, 2017

Conversation

Projects
None yet
2 participants
@AgustinCB
Contributor

AgustinCB commented Nov 16, 2017

Problem: the test builtin is falsely passing when one of the numbers is negative.

Solution: Two problems were addressed:

  • It was assuming that negative numbers were flag strings: Solved by checking that the second character is alphabetic.
  • It was parsing to usize. Solved by parsing to isize.

Changes introduced by this pull request:

Other than what commented in the solution, added tests for this cases.

TODOs: In the issue was commented the possibility of using decimal and parsing to d128. I didn't feel comfortable adding that overhead without a deeper discussion, so I just made the minimal fix to solve the problem.

Also... is is_alphabetic a good match? I felt tempted to use is_ascii_alphabetic instead, but it's still experimental and it wasn't considered a Pattern, so the compiler complained. is_alphabetical cover more cases than we do right now, so maybe is worth to use our own is_ascii_alphabetic?

Fixes: #586

State: Ready

@mmstick

This comment has been minimized.

Show comment
Hide comment
@mmstick

mmstick Nov 16, 2017

Collaborator

Thanks!

Collaborator

mmstick commented Nov 16, 2017

Thanks!

@mmstick mmstick merged commit 990a35d into redox-os:master Nov 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment