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

Allow leading and trailing whitespace in number strings #5620

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Jan 21, 2018

Followup to #5589.

@RX14
Copy link
Contributor

RX14 commented Jan 21, 2018

We typically have options for these, going from the String#to_i args.

@Sija
Copy link
Contributor Author

Sija commented Jan 21, 2018

@RX14 I know but decided against it, because String#to_*methods are implemented directly within String, and Big* classes are re-opening String with their own #to_big_* impl - which means that additional options needs to be passed along to appropriate constructors, and none of them are implemented ATM so I'd suggest leaving it as an enhancement for a future PR.

@ysbaddaden
Copy link
Contributor

You do implement the skip, using strip, so you can implement the options to allow whitespace or not.

@bcardiff
Copy link
Member

If we are relaxing the treating of whitespaces we should probably allow them between the sign and the number itself. Please ensure the same rules applies for non-big numbers. If custom logic is used to ignore whitspaces we should ensure (adding specs) that some other whitspace than 0x20 is treated as whitespace.

@RX14
Copy link
Contributor

RX14 commented Jan 22, 2018

Plus, all these string copies are expensive.

@straight-shoota
Copy link
Member

I see a valid use case for parsing numbers with stripping whitespace.

  • But the implementation needs to be configurable. I'd be fine with making that behaviour default, but there should be an option to control it.
  • The current implementation is not very performant because it creates string copies. Limiting the char array passed to the C function would avoid additional allocations.
  • Non-Big number types should also be covered.
  • In the current state, it's better to just call strip explicitly when you need this behaviour.

I propose to close this PR. If someone comes up with a new proposal, I'd be happy to accept it. But it's not a pressing issue, since the alternative is trivial.

@Sija Sija closed this Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants