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

Move from @require to v1.9 Ext #276

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Conversation

hustf
Copy link
Contributor

@hustf hustf commented Oct 10, 2023

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:

	modified:   Project.toml
	modified:   appveyor.yml
	new file:   ext/LuxorExt.jl
	renamed:    src/latex.jl -> ext/latex.jl
	modified:   src/Luxor.jl
 ERROR: LoadError: Dependency `MathTeXEngine` in target `test` not
 listed in `deps` or `extras` section.
modified:   Project.toml
@hustf
Copy link
Contributor Author

hustf commented Oct 10, 2023

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
Copy link
Contributor Author

hustf commented Oct 10, 2023

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!

modified:   .github/workflows/ci.yml
modified:   Project.toml
renamed:    ext/LuxorExt.jl -> ext/LuxorExtLatex.jl
modified:   ext/latex.jl
modified:   src/Luxor.jl
new file:   src/placeholders_for_extensions.jl
modified:   test/latex.jl
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

All modified lines are covered by tests ✅

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

Additional details and impacted files
@@            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 Coverage Δ
ext/LuxorExtLatex.jl 100.00% <100.00%> (ø)
ext/latex.jl 92.13% <ø> (ø)
src/Luxor.jl 100.00% <ø> (ø)
src/placeholders_for_extensions.jl 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cormullion
Copy link
Member

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

@hustf
Copy link
Contributor Author

hustf commented Oct 11, 2023

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
Copy link
Member

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
Copy link
Contributor Author

hustf commented Oct 11, 2023

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
Copy link
Member

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
Copy link
Contributor Author

hustf commented Oct 12, 2023

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")

)

@cormullion cormullion merged commit 06a6d02 into JuliaGraphics:master Oct 13, 2023
12 of 13 checks passed
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.

2 participants