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

float() / float_s() / double() / double_s(): Accept numbers without decimals #448

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@federicomenaquintero

federicomenaquintero commented Feb 22, 2017

Fixes #437

We add more detailed tests, and add support for floating-point numbers
that are written without decimals.

float() / float_s() / double() / double_s(): Accept numbers without d…
…ecimals

Fixes #437

We add more detailed tests, and add support for floating-point numbers
that are written without decimals.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 22, 2017

Coverage Status

Coverage decreased (-82.5%) to 0.0% when pulling c5733d4 on federicomenaquintero:master into fd9674c on Geal:master.

coveralls commented Feb 22, 2017

Coverage Status

Coverage decreased (-82.5%) to 0.0% when pulling c5733d4 on federicomenaquintero:master into fd9674c on Geal:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 22, 2017

Coverage Status

Coverage decreased (-82.5%) to 0.0% when pulling c5733d4 on federicomenaquintero:master into fd9674c on Geal:master.

coveralls commented Feb 22, 2017

Coverage Status

Coverage decreased (-82.5%) to 0.0% when pulling c5733d4 on federicomenaquintero:master into fd9674c on Geal:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 22, 2017

Coverage Status

Coverage decreased (-82.5%) to 0.0% when pulling c5733d4 on federicomenaquintero:master into fd9674c on Geal:master.

coveralls commented Feb 22, 2017

Coverage Status

Coverage decreased (-82.5%) to 0.0% when pulling c5733d4 on federicomenaquintero:master into fd9674c on Geal:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 22, 2017

Coverage Status

Coverage decreased (-82.5%) to 0.0% when pulling c5733d4 on federicomenaquintero:master into fd9674c on Geal:master.

coveralls commented Feb 22, 2017

Coverage Status

Coverage decreased (-82.5%) to 0.0% when pulling c5733d4 on federicomenaquintero:master into fd9674c on Geal:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 22, 2017

Coverage Status

Coverage decreased (-82.5%) to 0.0% when pulling c5733d4 on federicomenaquintero:master into fd9674c on Geal:master.

coveralls commented Feb 22, 2017

Coverage Status

Coverage decreased (-82.5%) to 0.0% when pulling c5733d4 on federicomenaquintero:master into fd9674c on Geal:master.

alt_complete!(
delimited!(opt!(digit), tag!("."), digit)
| preceded!(digit, tag!("."))
| digit

This comment has been minimized.

@sdroege

sdroege Feb 22, 2017

Contributor

That's basically the same as https://github.com/Geal/nom/blob/master/benches/json.rs#L27 now, just as a general comment. Change looks good to me!

@sdroege

sdroege Feb 22, 2017

Contributor

That's basically the same as https://github.com/Geal/nom/blob/master/benches/json.rs#L27 now, just as a general comment. Change looks good to me!

alt!(
delimited!(digit, tag!("."), opt!(digit))
| delimited!(opt!(digit), tag!("."), digit)
alt_complete!(

This comment has been minimized.

@Geal

Geal Feb 24, 2017

Owner

using alt_complete! can break the behaviour of the parser. If the data is123.4 but you get it as 123 then .4 (because of buffering, network packets or other reasons), you would parse it as 123 then a parser that fails on the unexpected .4.

I used it in benches/json.rs for convenience, but in the general case it does not work :/

@Geal

Geal Feb 24, 2017

Owner

using alt_complete! can break the behaviour of the parser. If the data is123.4 but you get it as 123 then .4 (because of buffering, network packets or other reasons), you would parse it as 123 then a parser that fails on the unexpected .4.

I used it in benches/json.rs for convenience, but in the general case it does not work :/

This comment has been minimized.

@federicomenaquintero

federicomenaquintero Feb 24, 2017

What's the strategy to handle that case? How would one say, "I need one more character to see if it is part of the number, or if the production ends here"?

@federicomenaquintero

federicomenaquintero Feb 24, 2017

What's the strategy to handle that case? How would one say, "I need one more character to see if it is part of the number, or if the production ends here"?

This comment has been minimized.

@joshtriplett

joshtriplett Feb 24, 2017

Other streaming parsers I've seen, such as HTTP parsers, handle cases like this by returning an "I need more data" indication to the caller (ideally with some idea of "how much more data"), which has to obtain more data and call back into the parser.

@joshtriplett

joshtriplett Feb 24, 2017

Other streaming parsers I've seen, such as HTTP parsers, handle cases like this by returning an "I need more data" indication to the caller (ideally with some idea of "how much more data"), which has to obtain more data and call back into the parser.

This comment has been minimized.

@federicomenaquintero

federicomenaquintero Feb 24, 2017

Nom already does that with IResult::Incomplete. But in this case, the buffer ends and the parser is at a state where it could be finished: think of reading "123.45" at the end of the buffer, but the next characters (which are unavailable yet) are "67".

digit() already has a check for input_length == 0 when it starts right at the end of the buffer. Maybe if it runs out of buffer in the for loop it should also return Incomplete?

(What would happen after that, both for the "here's some more data" case and for the "nope, that's the only buffer we'll have" case?)

@federicomenaquintero

federicomenaquintero Feb 24, 2017

Nom already does that with IResult::Incomplete. But in this case, the buffer ends and the parser is at a state where it could be finished: think of reading "123.45" at the end of the buffer, but the next characters (which are unavailable yet) are "67".

digit() already has a check for input_length == 0 when it starts right at the end of the buffer. Maybe if it runs out of buffer in the for loop it should also return Incomplete?

(What would happen after that, both for the "here's some more data" case and for the "nope, that's the only buffer we'll have" case?)

This comment has been minimized.

@federicomenaquintero

federicomenaquintero Feb 24, 2017

Are there tests for streaming? Something like a test_parser (parser, "some data") that tests the parser with a single buffer with the whole string, and with a streaming wrapper that feeds it 1 byte at a time?

@federicomenaquintero

federicomenaquintero Feb 24, 2017

Are there tests for streaming? Something like a test_parser (parser, "some data") that tests the parser with a single buffer with the whole string, and with a streaming wrapper that feeds it 1 byte at a time?

This comment has been minimized.

@sdroege

sdroege Feb 24, 2017

Contributor

Probably something with eof. Your parser wants more digits until eof or a non-digit is found

@sdroege

sdroege Feb 24, 2017

Contributor

Probably something with eof. Your parser wants more digits until eof or a non-digit is found

This comment has been minimized.

@Geal

Geal Jul 17, 2017

Owner

ok, so, anyone want to finish this PR? :)

@Geal

Geal Jul 17, 2017

Owner

ok, so, anyone want to finish this PR? :)

This comment has been minimized.

@richardwhiuk

richardwhiuk Oct 29, 2017

Should the parser differentiate between IncompleteOrDone or IncompleteOrError - which would allow the complete! macro to resolve it?

@richardwhiuk

richardwhiuk Oct 29, 2017

Should the parser differentiate between IncompleteOrDone or IncompleteOrError - which would allow the complete! macro to resolve it?

@marshallpierce

This comment has been minimized.

Show comment
Hide comment
@marshallpierce

marshallpierce Oct 26, 2017

I was just bitten by this problem; is it still in need of someone to take it over?

marshallpierce commented Oct 26, 2017

I was just bitten by this problem; is it still in need of someone to take it over?

@Geal Geal added this to the 4.0 milestone Nov 26, 2017

@Geal

This comment has been minimized.

Show comment
Hide comment
@Geal

Geal Jan 14, 2018

Owner

I think we can close this now, with the code that landed in 99e4cea

Owner

Geal commented Jan 14, 2018

I think we can close this now, with the code that landed in 99e4cea

@Geal Geal added the needs testing label Jan 14, 2018

@Geal Geal closed this May 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment