-
Notifications
You must be signed in to change notification settings - Fork 25
Improve @u_str
implementation to fix downstream precompilation
#106
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
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
@YingboMa what do you think? This completely avoids any I also removed the limitation of type requirement. So you can do things like @gaurav-arya are you okay with this? I think we might have to remove that type requirement to fix precompilation working in downstream packages, since evaluating types requires |
@u_str
implementation to fix downstream precompilation
10807b8
to
8b22c1e
Compare
Since this is quite a significant change in the parsing I'm going to sit on this PR for a bit in case I think of anything wrong with it. @gaurav-arya if you could take a look also that would be amazing |
For backwards compatibility we could have It's not 1:1 with Unitful then but at least it preserves the type stability. Like |
Okay I convinced myself the other way. I think This is unlike Unitful, but is like the current implementation of DynamicQuantities. So with this change it's no longer breaking. |
ff2c0f1
to
519356b
Compare
Woo! 100% test coverage! Alright we're good now. I'm merging as it's no longer a breaking change. @gaurav-arya if you did have a comment on the behavior of @YingboMa please confirm that precompilation works on v0.11.2 and feel free to add to the stdlib now. |
Fixes #105 which reported precompilation issues.
u_str
to avoid callingeval
(which breaks precompilation)us_str
NoDims
type so thatdimension(1.0)
doesn't throw a method error.uconvert
in more contexts, likeSymbolicDimensions
->SymbolicDimensions