JuliaGraphics / Luxor.jl

Simple drawings using vector graphics; Cairo "for tourists!"
http://juliagraphics.github.io/Luxor.jl/
Other
588 stars 71 forks source link

Move from @require to v1.9 Ext #276

Closed hustf closed 11 months ago

hustf commented 12 months ago
modified:   Project.toml
modified:   appveyor.yml
new file:   ext/LuxorExt.jl
renamed:    src/latex.jl -> ext/latex.jl
modified:   src/Luxor.jl

Ref. issue #275. Also, KristofferC talking, and his template with a few extra bells:

hustf commented 12 months ago

Hm, I changed appveyor.yml from Julia 1.6 to 1.9. This is still testing on 1.9 now; I suppose that is configured somewhere I can't access.

hustf commented 12 months ago

We define three functions for export from Luxor. The lecture demonstrates extending existing functions. We try something else here, which don't work. So one option is to define placeholder functions with general types in Luxor.jl, and then extend / specialize in LuxorExt.jl. For example, in Luxor:

latextextsize(x) = throw(ArgumentError("latextextsize can only be called with type LaTeXString from package `LatexStrings.jl`"))

The properly defined function returns a tuple of numbers, so not throwing an error would introduce type instability. (not that we care much about that). I'm leaving this for later, in case anybody has a comment already. In the meantime, feel free to take over at any time!

codecov[bot] commented 11 months ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (b508156) 74.62% compared to head (ba5f199) 74.45%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #276 +/- ## ========================================== - Coverage 74.62% 74.45% -0.18% ========================================== Files 33 35 +2 Lines 6691 6701 +10 ========================================== - Hits 4993 4989 -4 - Misses 1698 1712 +14 ``` | [Files](https://app.codecov.io/gh/JuliaGraphics/Luxor.jl/pull/276?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics) | Coverage Δ | | |---|---|---| | [ext/LuxorExtLatex.jl](https://app.codecov.io/gh/JuliaGraphics/Luxor.jl/pull/276?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-ZXh0L0x1eG9yRXh0TGF0ZXguamw=) | `100.00% <100.00%> (ø)` | | | [ext/latex.jl](https://app.codecov.io/gh/JuliaGraphics/Luxor.jl/pull/276?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-ZXh0L2xhdGV4Lmps) | `92.13% <ø> (ø)` | | | [src/Luxor.jl](https://app.codecov.io/gh/JuliaGraphics/Luxor.jl/pull/276?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-c3JjL0x1eG9yLmps) | `100.00% <ø> (ø)` | | | [src/placeholders\_for\_extensions.jl](https://app.codecov.io/gh/JuliaGraphics/Luxor.jl/pull/276?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics#diff-c3JjL3BsYWNlaG9sZGVyc19mb3JfZXh0ZW5zaW9ucy5qbA==) | `100.00% <100.00%> (ø)` | | ... and [4 files with indirect coverage changes](https://app.codecov.io/gh/JuliaGraphics/Luxor.jl/pull/276/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGraphics)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cormullion commented 11 months ago

Thanks! It looks more complicated than I thought at first... I'll try it out soon and see how it works.

hustf commented 11 months ago

I believe it will be hard to hit the extension code loading lines with test coverage. So I consider this finished. It should be completely non-breaking; no interface changes at all.

cormullion commented 11 months ago

According to this https://github.com/cjdoris/PackageExtensionCompat.jl - and https://discourse.julialang.org/t/package-extensions-for-julia-1-9/93397/9 - it's 'easy' to use requires at the same time as package extensions, so it might be possible to easily obtain greater compatibility.🤔

hustf commented 11 months ago

It is possible, yes! And the template has a relevant example. I personally prefer to cut down on the complexity and not put effort into supporting old versions. To me, the point is simplicity, clearer compartmentalisation and improved compile times. If we put in the code for compatibility, the simplicity at least is reduced.

Testing on Julia 1.6 and 1.9 requires a little more energy and time per commit.

So I guess my personal freference is still to drop the compatibility effort, and rather wait (for example until Julia 2.0) before reaping the benefits of the new system and fully drop Julia pre 2.0. Or to simply drop Julia pre v1.9 now.

(edit: there's even an authority that may seem to agree!)

cormullion commented 11 months ago

Thanks for all your hard work! I think we should go for a 3.9 release with a increased Julia requirement of v1.9...

But I remember that this all started with a suggested decoupling of FFMPEG... :) Is that easy to add to this PR?

hustf commented 11 months ago

I'm glad we think along the same lines!

I believe it would be benefical to separate FFMPEG in it's own PR. I don't feel confident to take longer steps, this being my first time and all.

Installing FFMPEG as a requirement to running some function could be considered a breaking change. Old code (parts of it mine) would stop working until users installed and loaded FFMPEG in their environment.

I haven't modified the documentation either. This PR probably motivates no documentation changes, while the added step of loading FFMPEG at need would.

(edit: to be clear, if I, before sepearting out FFMPEG, had an environment with a Project.toml that said

[deps]
ColorSchemes = "35d6a980-a343-548e-a6ea-1d62b119f2f4"
Dates = "ade2ca70-3891-5945-98fb-dc099432e06a"
Luxor = "ae8d54c2-7ccd-5906-9d76-62fc9837b5bc"

I would now be able to animate away. After separarating out FFMPEG, I would have to add

FFMPEG = "c87230d0-a227-11e9-1b43-d7ebe4e7570a"

or from the Julia prompt:

import Pkg; Pkg.add("FFMPEG")

)