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

Add package extension on RecipesBase #143

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

evetion
Copy link
Member

@evetion evetion commented Jul 12, 2024

This moves all the plots related code, including the macro(!) into GeoInterface. This now works:

using GeoInterface, Plots
Precompiling GeoInterface
  1 dependency successfully precompiled in 1 seconds. 1 already precompiled.
[ Info: Precompiling RecipesBaseExt [b9601432-96b0-576e-9c4b-47fc12d2b5bf]
points = GeoInterface.Point.(rand(10),rand(10))
julia> plot(points)
Screenshot 2024-07-12 at 10 28 16

Since we now export the macros, I think the existing GeoInterfacesRecipes continues to work, even though it's now empty.

Project.toml Outdated Show resolved Hide resolved
Comment on lines +144 to +151
macro enable(typ)
esc(expr_enable(typ))
end

# Compat
macro enable_geo_plots(typ)
esc(expr_enable(typ))
end
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this is no longer available as a "name" to import from GeoInterface / some other package. Do you want to keep the package GeoInterfaceRecipes and just load that code path as the extension?

Copy link
Member Author

Choose a reason for hiding this comment

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

The macro is exported by GeoInterface now, so it should be usable. I've answered your other question in the general comments.

@evetion
Copy link
Member Author

evetion commented Jul 12, 2024

This at least fixes #110 (for people that forget to do using GeoInterfaceRecipes).

I still have to check whether this can be used to deprecate GeoInterfaceRecipes completely, and how that would work. Essentially, as soon as a package uses RecipesBase, it would trigger this package extension, but if it triggers in an extension on RecipesBase, then the macro would not be available.

evetion and others added 2 commits July 12, 2024 11:39
Co-authored-by: Anshul Singhvi <anshulsinghvi@gmail.com>
@@ -0,0 +1,151 @@
# Convert coordinates to the form used by Plots.jl
_coordvecs(::PointTrait, geom) = [tuple(coordinates(geom)...)]
Copy link
Member

Choose a reason for hiding this comment

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

Can we put all these functions in the extension and just leave the stubs here to extend in the extension?

@asinghvi17
Copy link
Member

I'm also not sure about defining @enable as being only for Plots. Since RecipesBase is pretty small, (as is MakieCore), is there an issue with keeping the packages as they are, and simply using an include trick on GeoInterfaceRecipes for a package extension?

@evetion
Copy link
Member Author

evetion commented Jul 12, 2024

Yeah the other solution that I considered is just duplicating the code. I'm pretty unsure about include, as it's a standalone package, even though it's in a monorepo here right?

@rafaqz
Copy link
Member

rafaqz commented Jul 15, 2024

Can we just do the include("../GeoInterfaceRecipes/src/GeoInterfaceRecipes.jl") and include("../GeoInterfaceMakie/src/GoInterfaceMakie.jl") tricks in extensions GeoInterfaceRecipesExt and GeoInterfaceMakieExt?

Then we don't have the circular dep on the sub-packages but we can use the same code and not break anything.

It does end up using a bit more compilation and memory as its the same code used twice, but I think that's ok.

Edit: we may need to move code out of the modules into separate files for this to work.

@asinghvi17
Copy link
Member

GIMakieExt is circular but RecipesExt could work

@rafaqz
Copy link
Member

rafaqz commented Jul 15, 2024

Oh of course because Makie depends directly on GeoInterface.jl via GeometryOps.jl, forgot for a second.

Maybe... could we just move the GeometryBasics.jl geointerface code to an extension in GeoInterface.jl?

We need to solve this somehow

@asinghvi17
Copy link
Member

I think we tried to in a GeometryBasics PR but I ended up not wanting to do that for some reason. We can in theory just have Makie depend on GeoInterfaceMakie too...

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