Skip to content
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

support for 1.9 extensions #143

Merged
merged 34 commits into from May 15, 2023
Merged

Conversation

longemen3000
Copy link
Contributor

moves SpecialFunctions, Unitful and Juno support into extensions.

@giordano giordano linked an issue May 14, 2023 that may be closed by this pull request
Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this!

I'm also inclined towards removing the Juno stuff altogether: as far as I know it's broken, I doubt anyone uses it, Juno itself is discontinued. But this is besides the point of this PR.

ext/MeasurementsJunoExt.jl Outdated Show resolved Hide resolved
ext/MeasurementsJunoExt.jl Outdated Show resolved Hide resolved
ext/MeasurementsSpecialFunctionsExt.jl Outdated Show resolved Hide resolved
ext/MeasurementsSpecialFunctionsExt.jl Outdated Show resolved Hide resolved
ext/MeasurementsUnitfulExt.jl Outdated Show resolved Hide resolved
src/Measurements.jl Outdated Show resolved Hide resolved
src/show.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
longemen3000 and others added 8 commits May 13, 2023 20:09
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
@codecov
Copy link

codecov bot commented May 14, 2023

Codecov Report

Merging #143 (9c63073) into master (f1d6e89) will increase coverage by 1.28%.
The diff coverage is 25.00%.

❗ Current head 9c63073 differs from pull request most recent head cce870b. Consider uploading reports for the commit cce870b to get more accurate results

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
+ Coverage   95.63%   96.91%   +1.28%     
==========================================
  Files          12        9       -3     
  Lines         756      616     -140     
==========================================
- Hits          723      597     -126     
+ Misses         33       19      -14     
Impacted Files Coverage Δ
src/show.jl 100.00% <ø> (+16.66%) ⬆️
src/Measurements.jl 77.41% <25.00%> (-9.25%) ⬇️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@longemen3000
Copy link
Contributor Author

longemen3000 commented May 14, 2023

Finally, tests pass hahah. the __init__ function requires that particular arrangement to work on earlier versions (based on SciML/DiffEqBase.jl@c2de920) .
I think that there will be some problems with Aqua.jl afterwards, because there isn't any compat of the @required functions.

src/Measurements.jl Outdated Show resolved Hide resolved
Comment on lines 87 to 97
if !isdefined(Base, :get_extension)
using Requires
end

function __init__()
@require Unitful="1986cc42-f94f-5a68-af5c-568840ba703d" include("unitful.jl")
@require SpecialFunctions="276daf66-3868-5448-9aa4-cd146d93841b" include("special-functions.jl")
@static if !isdefined(Base, :get_extension)
@require Unitful="1986cc42-f94f-5a68-af5c-568840ba703d" include("../ext/MeasurementsUnitfulExt.jl")
@require SpecialFunctions="276daf66-3868-5448-9aa4-cd146d93841b" include("../ext/MeasurementsSpecialFunctionsExt.jl")
@require Juno="e5e0dc1b-0480-54bc-9374-aad01c23163d" include("../ext/MeasurementsJunoExt.jl")
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point just use

@static if !isdefined(Base, :get_extension)
    using Requires
    function __init__()
        @require Unitful="1986cc42-f94f-5a68-af5c-568840ba703d" include("../ext/MeasurementsUnitfulExt.jl")
        @require SpecialFunctions="276daf66-3868-5448-9aa4-cd146d93841b" include("../ext/MeasurementsSpecialFunctionsExt.jl")
        @require Juno="e5e0dc1b-0480-54bc-9374-aad01c23163d" include("../ext/MeasurementsJunoExt.jl")
    end
end

Having a duplicate if looks pointless and you avoid having an __init__ function completely when not needed.

test/runtests.jl Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@longemen3000
Copy link
Contributor Author

the loading of RecipesBase was done in a backwards-compatible (https://pkgdocs.julialang.org/dev/creating-packages/#Transition-from-normal-dependency-to-extension) but aqua complains about that (and is the reason the Aqua test fail)

test/runtests.jl Outdated Show resolved Hide resolved
@longemen3000
Copy link
Contributor Author

the tests are now explicitly failing for JuliaTesting/Aqua.jl#105 (the Pkg order is different than the order Aqua expects), should be fixed by JuliaTesting/Aqua.jl#106 ?

@giordano
Copy link
Member

Can't you just change the order to make the test pass or am I missing something?

@longemen3000
Copy link
Contributor Author

i will try, hope that a Pkg.resolve() does not change the proposed order

@longemen3000
Copy link
Contributor Author

Tests seems to pass (now with Aqua.jl)

ext/MeasurementsRecipesBaseExt.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
longemen3000 and others added 2 commits May 15, 2023 15:31
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
@giordano
Copy link
Member

Ok, I think this is in a good shape now. Thanks again for this!

@giordano giordano merged commit 0b54af0 into JuliaPhysics:master May 15, 2023
11 checks passed
if !isdefined(Base,:get_extension)
using Requires
using RecipesBase
include("../ext/MeasurementsRecipesBaseExt.jl")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional that only this single extension is outside of the next block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this package supports julia 1.0. for some reason, using one isdefined block failed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks.

@giordano giordano mentioned this pull request May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move to pkgextensions for Julia v1.9+
3 participants