Skip to content

Replace plottertypes by Singleton#109

Open
jpthiele wants to merge 1 commit into
mainfrom
jpt/singleton
Open

Replace plottertypes by Singleton#109
jpthiele wants to merge 1 commit into
mainfrom
jpt/singleton

Conversation

@jpthiele

Copy link
Copy Markdown
Contributor

As proposed in #108 this changes dispatch.jl to use Singletons instead of using the set of nested ifs in plottertype with ismakie() and similar.

Since MakieType was heuristic based I had to introduce the alternative AbstractMakieType and change the Makie extension accordingly by adding where {MakieType <: AbstractMakieType} where needed.
Then all tests ran locally without problem.

Now, extending GridVisualize with another plotting backend can be done without needing to touch the main package.
Nevertheless, introducing a new shorthand through const AnotherPlotterPackageType = PlotterType{:AnotherPlotterPackage}
is certainly nice and quickly done.

@j-fu do you know of any downstream package that might explicitly use MakieType?

And if you are fine with this we should test the alternative to the example to remove the !private! functions ismakie and similar.

@jpthiele

Copy link
Copy Markdown
Contributor Author

Had a quick search on GitHub and all that came up were forks of GridVisualize or completely unrelated projects when it comes to MakieType.

@j-fu

j-fu commented Dec 13, 2025

Copy link
Copy Markdown
Member

I think formally this would be a breaking change which by semver increases the major version number and we very much should try to stick to this. I would put this on a list with other possibly breaking changes, which together would be substantial enough to be worth a najor release.

@j-fu j-fu marked this pull request as draft December 13, 2025 03:04
@j-fu

j-fu commented Jun 4, 2026

Copy link
Copy Markdown
Member

Ok, had a closer look.

I was first surprised by the nameof approach (I wasn't really awareof this) , but this works, as with

MMM=Plots
nameof(MMM)

still would return :Plots.

@j-fu

j-fu commented Jun 4, 2026

Copy link
Copy Markdown
Member

The other surprise is the use of Union types here. Indeed, they give the correct subtype relations. However isabstracttype(Union{Int32,Int64})==false, and therefore it might be better to call them UnionMakieType etc.

@j-fu

j-fu commented Jun 4, 2026

Copy link
Copy Markdown
Member

For the formal semver consistency we could just set const MakieType = UnionMakieType and keep exporting this, with some deprecation information.

@j-fu

j-fu commented Jun 4, 2026

Copy link
Copy Markdown
Member

I think the PR then also should replace the implementations of the ispyplot etc. functions. These use the real dangerous heuristics, and they are used sometimes
(at least in my teaching notebooks...).

@jpthiele jpthiele force-pushed the jpt/singleton branch 2 times, most recently from 40ba322 to d87c6bb Compare June 16, 2026 14:07
@jpthiele

Copy link
Copy Markdown
Contributor Author

@j-fu I have rewritten the isxyz functions to use plottertype(Plotter) <: XYZType which should hopefully work, but it would be great if you could try that in one of your notebooks.
Since that requires additional manual labor for adding new types and the implementation is practically the same I have deprecated them with the hint to use that check instead.

@jpthiele jpthiele marked this pull request as ready for review June 16, 2026 14:08
@jpthiele

Copy link
Copy Markdown
Contributor Author

Also, in regards to #110 I have moved all the deprecated things to a dedicated deprecated.jl file in src, including their exports which should make the actual act of removing everything easier in a later breaking release.

@jpthiele jpthiele force-pushed the jpt/singleton branch 4 times, most recently from 62eeb74 to 7885bd8 Compare June 16, 2026 17:39
@j-fu

j-fu commented Jun 17, 2026

Copy link
Copy Markdown
Member

Ok I checked this out and test this in a couple of projects. So far, it works, and I think it should generally be ok.

However I think that we should not deprecate ismakie, ispyplot etc. We have implementations without heuristics now and we should avoid unnecessary breaking changes.

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