-
Notifications
You must be signed in to change notification settings - Fork 22
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
add LinearAlgebraExt #86
Conversation
Hey thanks. In principle I agree this is a good move. However we were burnt by something similar before JuliaObjects/Accessors.jl#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 JuliaLang/julia#52511) |
How would that work? Would you add |
i was thinking on just guarding the loading of extensions against |
Co-authored-by: Jan Weidner <[email protected]>
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 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. JuliaLang/julia#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: |
else | ||
import ..ConstructionBase | ||
import ..LinearAlgebra | ||
end |
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.
not needed?
src/nonstandard.jl
Outdated
end | ||
if !isdefined(Base,:get_extension) | ||
using LinearAlgebra | ||
include("../ext/ConstructionBaseLinearAlgebraExt.jl") |
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.
move to main source file?
src/nonstandard.jl
Outdated
throw(ArgumentError("Invalid patch for `Cholesky`: $(patch)")) | ||
end | ||
if !isdefined(Base,:get_extension) | ||
using LinearAlgebra |
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.
using
is not needed
…Base.jl into LinearAlgebraExt
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
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 ☔ View full report in Codecov by Sentry. |
i would say that this PR is ready to merge. as mentioned before, this PR no longer sets test errors are integration errors, that are also present on the master branch. |
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.
LGTM, thanks @longemen3000
should make the package dependency free on julia > 1.9. motivated by JuliaFolds2/Trand