Enable package extension for Statistics #277
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #277 +/- ##
=======================================
Coverage 97.05% 97.05%
=======================================
Files 6 7 +1
Lines 782 782
=======================================
Hits 759 759
Misses 23 23 ☔ View full report in Codecov by Sentry. |
|
This seems more elegant than the use of Requires.jl with respect to Stdlib. On the other hand, in the first place, this workaround uses a non-public function Seems to me we need to revisit the workaround itself. |
|
This change is not so complicated, and the intent is clear. |
|
Sorry, I missed this PR. Adding extension support is a good change, but it requires Julia 1.9 to work. For the very fundamental packages such as FixedPointNumbers, keeping a broad range of compatibility support is necessary. It's necessary to keep it compatible with the current LTS version, i.e., Julia 1.6. The current FixedPointNumbers is still compatible with Julia 1.0, which is more than great. Thus, we still need to preserve the old codes, still need to install Requires (but conditionally loading it depending on the actual Julia versions). Ref: https://pkgdocs.julialang.org/v1/creating-packages/#Backwards-compatibility |
|
@johnnychen94 You are absolutely right. But for the case of Statistics, it has been a stdlib and should work as before. (Conversely, the "new” Statistics.jl will basically not be introduced into the old julia.) |
Sorry, I misunderstood the code. The current version is compatible with old Julia versions by falling back to a direct
|
src/FixedPointNumbers.jl
Outdated
| if !isdefined(Base, :get_extension) | ||
| include("../ext/FixedPointNumbersStatisticsExt.jl") |
There was a problem hiding this comment.
Although this is in keeping with PR #255 manner, I would prefer that the precompile be at the end of the module (except for the __init__()).
There was a problem hiding this comment.
Actually, I'm not familiar with _precompile_() function. Perhaps the current code should be replaced with PrecompileTools.jl.
There was a problem hiding this comment.
Yes, it is from the SnoopCompile.jl days.
I think replacing before releasing the new version is a good idea.
|
@hyrodium Could you |
Project.toml
Outdated
| [compat] | ||
| Requires = "1" | ||
| julia = "1" |
There was a problem hiding this comment.
If we follow the principle, Statistics should be in [compat].
This is especially the case where even patch versions need to be included (e.g. Statistics = "1 - 1.11.1").
cf. #277 (comment)
There was a problem hiding this comment.
To my surprise, it seems that the hyphen notation is not supported for julia v1.0.
Sorry for misleading you.
Or "＜ 1.11.2"?
There was a problem hiding this comment.
Yeah, < 1.11.2 could be a solution (cf. Pkg.jl v1.0 documentation). However, "1 - 1.11.1" looks better to me because it has a lower bound. Is there any chance to drop support for Julia 1.0?
There was a problem hiding this comment.
Since Statistics.jl is stdlib, the lower bound is practically meaningless.
I don't intend to continue actively supporting julia v1.0, but I don't think it has caused any critical problems yet.
There was a problem hiding this comment.
Also, this is very trivial, the uppercase "S" of Statistics precedes the lowercase "j" of julia.
There was a problem hiding this comment.
Thanks for the comments! I have updated the compat bound for Statistics.jl
kimikage
changed the title
hyrodium
force-pushed
the
feature/package_extension
branch
from
407e84f to
8528da3
Compare
|
@hyrodium Thank you for rebasing. |
|
@hyrodium Thank you for addressing this issue! |
This PR reverts #255.