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

Adapted number regex to allow scientific notation #490

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

rhazn
Copy link
Contributor

@rhazn rhazn commented Dec 18, 2023

Allows parsing numbers in scientific notation, both integers like 1.904761905e+20 or decimals like 8.1e-5.

Closes #488

@rhazn rhazn added the softwarecampus Issues related to the Softwarecampus grant label Dec 18, 2023
@rhazn rhazn self-assigned this Dec 18, 2023
const DECIMAL_DOT_SEPARATOR_REGEX = /^[+-]?([0-9]*[.])?[0-9]+$/;

const INTEGER_REGEX = /^[+-]?[0-9]+$/;
const NUMBER_REGEX = /^[+-]?([0-9]*[,\\.])?[0-9]+([eE][+\\-]?\d+)?$/;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious; what does the [,\\.] do?
I only know this syntax here to indicate a logical "or": [,|.].
But I assume that is not what you want to express?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my knowledge the logical or exists, so ,|\. would be , or .. With the bracers, you define a group of characters that satisfy the regex, so [,\.] is "one of , or ." which is functionally identical to ,|\.. The difference starts with things like [0-9] that you could write as 0|1|2|3|4|5|6|7|8|9.

But since we have used [+-] before (and use [eE] later), I think sticking to these two character groups makes sense as well. Even though, seeing this I am not sure why I introduced the double escape in the second [+-].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't then that sub-expression mean "one of the following: , or \ or any character" since the backslash is escaped instead of the fullstop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I changed this again to not use the escaping character. I think (from the regex testers I used), the escape works as desired but it is just not needed in this case.

Copy link
Member

@georg-schwarz georg-schwarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we document somewhere which kinds of notations we support?

Comment on lines +55 to +59
/**
* Reuse decimal number parsing to capture valid scientific notation
* of integers like 5.3e3 = 5300. In contrast to decimal, if the final number
* is not a valid integer, returns undefined.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it!

@rhazn
Copy link
Contributor Author

rhazn commented Dec 19, 2023

Should we document somewhere which kinds of notations we support?

Probably, even though my hope is still that we will write parsers in Jayvee so they can be extended by users. I'd then move the notation documentation into their docs and autogenerate docs from there as we do for blocks. Otherwise I fear documentation will quickly get out of sync with implementation.

@rhazn rhazn force-pushed the issue-488-allow-scientific-notation branch from 0e2a24c to 8b68399 Compare December 19, 2023 10:18
@rhazn rhazn merged commit cee783e into main Dec 19, 2023
2 checks passed
@rhazn rhazn deleted the issue-488-allow-scientific-notation branch December 19, 2023 10:22
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
softwarecampus Issues related to the Softwarecampus grant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Scientific mathematical notations are being read as text
2 participants