-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: add Å to SI accepted units #1442
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.
New tests look good!
You should run npm run lint:fix
.
The PR title enforcement is kinda dumb imo, but you can change it to feat: add Å to SI accepted units
to comply.
src/common/utils.ts
Outdated
Unit.isValidAlpha = function (c) { | ||
return isAlphaOriginal(c) || c == 'Å' | ||
} | ||
createUnit('Å', '1 angstrom') |
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 works since mathjs is only imported in a single file. However if we need to handle units from multiple files then we should refactor this so that the module setup code only gets called once.
I changed the title and ran the lint; everything should be in order now. |
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.
Looks good to me.
Description
Added Å as accepted mathjs unit, equivalent to angstrom
Motivation
Required for the correct interpretation of certain metadata, can be easily extended for other special character units.
Fixes:
Please provide a list of the fixes implemented by this PR
Changes:
Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included