ConstructionBase.jl
ConstructionBase.jl copied to clipboard
add LinearAlgebraExt
Hey thanks. In principle I agree this is a good move. However we were burnt by something similar before https://github.com/JuliaObjects/Accessors.jl/issues/127
@aplavin you think this PR would trigger the same issues?
seems that the problem is (somewhat) solved in 1.11, so maybe a good idea would be to guard extensions against that release instead? (see https://github.com/JuliaLang/julia/issues/52511)
seems that the problem is (somewhat) solved in 1.11, so maybe a good idea would be to guard extensions against that release instead? (see JuliaLang/julia#52511)
How would that work? Would you add julia = "1.11" to ConstructionBase/Project.toml?
seems that the problem is (somewhat) solved in 1.11, so maybe a good idea would be to guard extensions against that release instead? (see JuliaLang/julia#52511)
How would that work? Would you add
julia = "1.11"toConstructionBase/Project.toml?
i was thinking on just guarding the loading of extensions against if Base.VERSION < v"1.11" instead of using if !isdefined(Base,:get_extension)
Guarding against 1.11 seems fine to me, I guess you won't be using the LTS in projects that need to trim LinearAlgebra.jl anyway.
Not sure what is the best way here. I'd suggest being totally sure a foundational package like this doesn't throw any warnings on installation/loading in at least 1.10 and newer. Test with running julia afresh as JULIA_DEPOT_PATH=$(mktemp -d) julia and installing it.
I understand it's potentially frustrating, but having a smooth user experience without confusing warnings for everyone is better than shaving a few stdlib dependencies. https://github.com/JuliaLang/julia/issues/52511 isn't fully fixed even on 1.11.
What about just having the structure ready for when the problem is solved?, I suppose that this problem will be solved in a future 1.10 release and a 1.11 one, so it is just a matter of waiting for those releases, and loading the ext unconditionally until then?
Yeah, that's what we ended up doing in Accessors for now: https://github.com/JuliaObjects/Accessors.jl/blob/270e78fed32763096c448541148d520aa275e667/src/Accessors.jl#L19-L22 and https://github.com/JuliaObjects/Accessors.jl/blob/master/ext/AccessorsDatesExt.jl
:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.
Codecov Report
Attention: Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.
Project coverage is 2.00%. Comparing base (
8e3e773) to head (6947d65). Report is 10 commits behind head on master.
| Files | Patch % | Lines |
|---|---|---|
| ext/ConstructionBaseLinearAlgebraExt.jl | 0.00% | 18 Missing :warning: |
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:exclamation: There is a different number of reports uploaded between BASE (8e3e773) and HEAD (6947d65). Click for more details.
HEAD has 20 uploads less than BASE
Flag BASE (8e3e773) HEAD (6947d65) 22 2
Additional details and impacted files
@@ Coverage Diff @@
## master #86 +/- ##
==========================================
- Coverage 25.49% 2.00% -23.50%
==========================================
Files 5 6 +1
Lines 153 150 -3
==========================================
- Hits 39 3 -36
- Misses 114 147 +33
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
i would say that this PR is ready to merge. as mentioned before, this PR no longer sets LinearAlgebra as an extension, but it paves the way to allow that change.
test errors are integration errors, that are also present on the master branch.