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

🔢 Support for scientific notation and decimal numbers in si role #577

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jan-david-fischbach
Copy link
Contributor

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

Copy link
Collaborator

@fwkoch fwkoch left a 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
Copy link
Collaborator

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 🤷‍♀️

if (!match) {
return [{ type: 'si', error: true, value }];
}
const [, number, commands] = match;
const number = match[1];
const commands = match[match.length-1];
Copy link
Collaborator

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.

@agoose77
Copy link
Contributor

@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.

@agoose77 agoose77 requested a review from rowanc1 January 29, 2024 19:35
@agoose77
Copy link
Contributor

@fwkoch thinking of trying to unblock things, is this PR now good to go?

@agoose77 agoose77 changed the title si role: support for scientific notation and decimal numbers ENH: support for scientific notation and decimal numbers in si role Mar 19, 2024
@agoose77
Copy link
Contributor

@rowanc1 we should do something about this. Is there a reason to consider it blocked, or is it mergeable?

@agoose77 agoose77 changed the title ENH: support for scientific notation and decimal numbers in si role 🔢 Support for scientific notation and decimal numbers in si role Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants