-
Notifications
You must be signed in to change notification settings - Fork 65
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
🔢 Support for scientific notation and decimal numbers in si
role
#577
base: main
Are you sure you want to change the base?
🔢 Support for scientific notation and decimal numbers in si
role
#577
Conversation
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.
@jan-david-fischbach Thanks, this is great! Do you mind running prettier
and fixing up the linting error CI is catching (as well as adding a couple more tests from my other comment)? Hopefully that's all easy enough, let me know if I can help!
@@ -32,3 +32,22 @@ cases: | |||
unit: Hz | |||
units: [hertz] | |||
value: 100 Hz | |||
|
|||
|
|||
- title: decimal number parses |
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.
Can you add a couple more test cases (hopefully easy copy/paste)? At least one with a negative, maybe one with only a decimal, no exponent?
Hmm, I think .345 <\hertz>
might fail still... That's totally fine with me, just another case to consider (maybe add a failing case for this one with error: true
if you agree that failing is the right behaviour). Could also add a failing case for point-delimited numbers, eg. 1.000.000,05
🤷♀️
packages/myst-roles/src/si.ts
Outdated
if (!match) { | ||
return [{ type: 'si', error: true, value }]; | ||
} | ||
const [, number, commands] = match; | ||
const number = match[1]; | ||
const commands = match[match.length-1]; |
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.
👍 This makes sense, necessary now that there are a bunch more groups in the regex.
bf55487
to
0568e12
Compare
@fwkoch I updated this PR as I thought it would be a good place to start looking at roles. Would you be happy to give this another review? I haven't given much thought to what the renderer should do with this AST yet, e.g. use KaTeX to render the math text. |
@fwkoch thinking of trying to unblock things, is this PR now good to go? |
si
role: support for scientific notation and decimal numberssi
role
@rowanc1 we should do something about this. Is there a reason to consider it |
si
rolesi
role
So far the
si
role only supported integer numbers. With this PR it becomes possible to use decimal and scientific "e" notation. Additionally a negative sign "-" at the start is also accepted