-
Notifications
You must be signed in to change notification settings - Fork 122
Declare public symbols #811
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #811 +/- ##
=========================================
Coverage ? 88.90%
=========================================
Files ? 21
Lines ? 1722
Branches ? 0
=========================================
Hits ? 1531
Misses ? 191
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/public.julia
Outdated
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.
- Why
.juliaextension instead of.jl? - This file is simply unmaintainable. Adding an option to macros like
@unit/@dimension/@derived_dimensionto make the symbol public looks a much more sustainable solution going forward
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.
Adding an option to macros
I agree, and would look into a macro solution.
Independent on that: Before I continue, I'd prefer to know whether the the variables sorting into private and public, as currently implemented in public.julia is OK, or needs any changes.
May I ask the maintainers to have a look?
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.
I would declare the following as public:
- all physical constants,
- everything defined through
@dimension/@derived_dimension, AbstractQuantity,- all non-logarithmic units (most of them are defined through
@refunit/@unit, but some, likehaandÅ, will have to be treated manually). For@affineunit, adding an extra argument to the macro has less of an advantage, since it only defines one symbol, but it could be done just to be consistent with@refunit/@unit.
Those are all the symbols I would declare public right now. Others can be added later.
I’m not sure about the logarithmic units. I wouldn’t declare them public for now, since I consider them broken in a way that cannot be fixed without major (and breaking) changes, but on the other hand, they can be accesssed through u"…" and are widely used (I think).
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.
@sostock On log units: They are intended for the package user, not for some internal purposes. Therefore IMO they should be declared public, too. I'd suggest adding a warning to the docstrings about their experimental nature.
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.
Another question: @dimension returns the dimension, @unit returns the unit, whereas @derived_dimension returns nothing. If that is intended, what's the reason? Otherwise I'd change @derived_dimension to return the dimension, too
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.
@dimension 𝐋 "𝐋" Length true returns Unitful.𝐋 (not Unitful.Length), so if @derived_dimension Area 𝐋^2 true returned something, it should return 𝐋^2, anything else would be inconsistent. But the value 𝐋^2 is not bound to any symbol by the macro, so I can see why there is no use in returning it.
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.
everything defined through @dimension/@derived_dimension
Each @dimension defines four bindings, eg. for Lenth we create and declare as public
𝐋, Length, LengthUnits, LengthFreeUnits. My current macro implementation makes all them public - but I'm not sure it is what you meant, or was it just the first two of them.
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.
My current macro implementation makes all them public
That’s what I would do as well.
b7fd95f to
2e41dae
Compare
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.
I haven’t reviewed the code yet, these comments are just about the default value of the new argument and the docstrings.
Co-authored-by: Sebastian Stock <[email protected]>
src/pkgdefaults.jl
Outdated
| \nDimension: [`Unitful.𝐌`](@ref). | ||
| \nSee Also: [`Unitful.kg`](@ref)." | ||
| @unit u "u" UnifiedAtomicMassUnit 1.660_539_066_60e-27*kg false # (50) | ||
| @public u |
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.
Is there a reason why the new macro argument isn’t used here?
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.
changed to use macro argument
|
I'd also suggest to declare |
Declaration of public symbols as requested in #749
Notes:
public.julialists non-public symbols, too. That is for maintainers to have an overview, can be removed later, of course.public.juliahas non-standard suffix because I remember having some issues with.jlsuffix in such a case. It could be CodeCov, not sure. Can be changed to.jlif desired.public.juliawas generated.testsetto check if all variables are declared in one way or another. Thistestsetis put it to the top ofruntest.jlbecause I got errors elswhere (see below).VelocityFreeUnitsaspublic(or exported, for that case) leads to failure of the "Display" testset. No idea why, but reproduced on Mac and Ubuntu. For debugging, see https://github.com/Eben60/Unitful.jl/tree/mark_public_units_debug