ENH: support for scientific notation and decimal numbers in si role
#577
Conversation
There was a problem hiding this comment.
@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.
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.
👍 This makes sense, necessary now that there are a bunch more groups in the regex.
agoose77
force-pushed
the
siunitx-role
branch
from
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? |
agoose77
changed the title
si role: support for scientific notation and decimal numberssi role
So far the
sirole 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