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

Update Unitful.uparse() #639

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michikawa07
Copy link
Contributor

@michikawa07 michikawa07 commented Mar 29, 2023

1 default value of unit_context argument
Nevertheless, expanded pkg of Unitful.jl (i.e., UnitfulAtomic.jl) execute Unitful.register(), Unitful.uparse did not recognize these pkgs automatically (due to the defaut value of unit_context arg is Unitful only).
This commit modified this issue.

2 allowed_funcs
To enable parsing some strings such as "(1:10)m", I added :colon and :(:).

3 parsing macro call
To enable parsing some strings such as "u"m/s"" (unit string including @u_str macro), I added some code in uparse content

Nevertheless expanded pkg of Unitful.jl (i.e., UnitfulAtomic.jl) execute Unitful.register(), Unitful.uparse did not recognize these pkgs automaticaly (due to the defaut value of unit_context arg is Unitful only).
This commit modified this issue.
@michikawa07 michikawa07 changed the title Update default value of unit_context arg in uparse Update Unitful.uparse() Mar 29, 2023
@@ -657,12 +657,12 @@ julia> uparse("1.0*dB")
1.0 dB
```
"""
function uparse(str; unit_context=Unitful)
function uparse(str; unit_context=vcat(Unitful, Unitful.unitmodules))
Copy link

Choose a reason for hiding this comment

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

+1 for this change, I just spent way too long trying to figure out why the imported unit was not parsing, the error message of lookup_units also was not super helpful

@sostock
Copy link
Collaborator

sostock commented Dec 2, 2023

1 default value of unit_context argument

IMO, this is a good change.

2 allowed_funcs
To enable parsing some strings such as "(1:10)m", I added :colon and :(:).

We could think about this, but I would rather keep the number of allowed functions small. If (1:3)m is allowed, why not [1,2,3]m? What’s so special about :?

3 parsing macro call
To enable parsing some strings such as "u"m/s"" (unit string including @u_str macro), I added some code in uparse content

I don’t see how this is useful. Where would a “nested” unit string appear?

@michikawa07
Copy link
Contributor Author

michikawa07 commented Dec 5, 2023

Thank you for your feedback.

We could think about this, but I would rather keep the number of allowed functions small. If (1:3)m is allowed, why not [1,2,3]m? What’s so special about :?

Hmmm, that may be true. If uparse allows ":", it should also allow "[]", and then there are no limits.

I don’t see how this is useful. Where would a “nested” unit string appear?

Yes I want use the nested unit sting. When saving a number with units, it may be converted to a macro and then written out as a string.

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