Skip to content

Conversation

@rafaqz
Copy link
Member

@rafaqz rafaqz commented May 20, 2025

This PR moves MakieCore and GeometryBasics to an extension. Resolving JuliaGeo/Shapefile.jl#139

Its a little more annoying than it could be because extending the macro in the extension seems to have world age issues - the extensions wont see the new methods added even if you can see them in the REPL before they load!

But, instead of putting the macro in an extension we can just keep it in the package and make a breaking change.

In the calling package we just add Makie or MakieCore manually:

GeoInterfaceMakie.@enable MakieCore Shapefile.AbstractShape

And it all seems to work fine, and MakieCore/GeometryBasics dependencies are now optional.

@rafaqz
Copy link
Member Author

rafaqz commented May 20, 2025

Uggh we have a loop with LibGEOS in the tests. Will have to update that and test off a branch. Pobably good to show that it works (in LibGEOS) anyway...

@evetion
Copy link
Member

evetion commented May 20, 2025

Can you explain why the two argument macro is now needed?

Note that in #143 we had an approach to rename and move the macros @enable_plots and @enable_makie to GeoInterface.jl stubs.

@rafaqz
Copy link
Member Author

rafaqz commented May 20, 2025

We would need to move the Geometry basics GeoInterface extension into GeoInterface to avoid the dependency loop. But yes then your strategy can work.

But I think it will need the same double argument macro: the reason is it seems that we cant add methods to a macro in an extension that get used from another extension.

Adding a parameter for it let's the package just pass MakieCore it in so there is no dependencies needed for the macro part. Maybe you solved that another way but it seemed difficult to me.

I think it's just internal problem of how extending macros works in extensions, I guess because they need to be loaded earlier than functions.

(So should we just do the GeometryBasics extension move then update your PR?)

@rafaqz
Copy link
Member Author

rafaqz commented May 20, 2025

JuliaGeometry/GeometryBasics.jl#221

The problem is then geointerface_geomtype which we can add module dispatch to fix now 1.10 is LTS.

Quite doable, just a few fiddly 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.

3 participants