-
Notifications
You must be signed in to change notification settings - Fork 51
A decimal exponent is not required for a number #204
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
A decimal exponent is not required for a number #204
Conversation
This ensures that a decimal exponent is optional
@@ -120,7 +120,7 @@ number = | |||
numberRegex :: forall m. ParserT String m String | |||
numberRegex = either unsafeCrashWith identity $ regex pattern mempty | |||
where | |||
pattern = "[+-]?[0-9]*(\\.[0-9]*)?([eE][+-]?[0-9]*(\\.[0-9]*))?" | |||
pattern = "[+-]?[0-9]*(\\.[0-9]*)?([eE][+-]?[0-9]*(\\.[0-9]*)?)?" |
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.
Since this makes everything optional, won't this also successfully parse things that aren't numbers (e.g. "foo
)?
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.
It makes the group (\\.[0-9]*)
optional. So only a decimal point followed by 0 or more decimal digits becomes optional.
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.
[+-]? -- these chars are optional
[0-9]* -- there can be 0 or more of these chars
(\\.[0-9]*)? -- these chars can be optional
(
[eE] -- not optional
[+-]? -- optional
[0-9]* -- 0 or more chars
(\\.[0-9]*)? -- now optional (previously wasn't)
)? -- Wait! This entire block is already optional!?
Nevermind, I think my issue is with the regex as a whole since all parts are optional.
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.
Huh, that is a good point. There should probably be an "or" somewhere early on so that it matches either a digit or a .
followed by a digit at a minimum.
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.
It's really hard to write a regex that admits all legal number strings but no other strings, but luckily that's not what we're doing here. We only need to find the number substring boundary so that we can pass the substring to the Data.Number.fromString
function. So
- We need to admit all number strings. That's the first importance, and this PR fixes that.
- It's okay if we admit some string that are not number strings. They'll be rejected by
Data.Number.fromString
.
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.
Ah okay, that totally makes sense! I probably should have looked outside the context of this regex.
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.
Yeah, thanks for clarifying that!
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.
👍
Thanks for the PR! I'll merge. |
I'll make a release too. |
I made a release, thanks again @MaybeJustJames |
This ensures that a decimal exponent is optional in the
Parsing.String.Basic.number
parser. Fixes #203.Checklist: