Skip to content

Conversation

@juliohm
Copy link
Member

@juliohm juliohm commented May 20, 2025

Fix #138

@juliohm juliohm requested a review from visr May 20, 2025 01:21
@rafaqz
Copy link
Member

rafaqz commented May 20, 2025

Most JuliaGeo geometry packages can plot their objects in Makie now after a lot of complaints and confusion that they didn't.

MakieCore is not particularly large but I totally agree making it a weak dep would be better.

But we need to find a way for that to continue working with a weak dependency, not just removing all of the code and breaking user expectation that plot(shapefilegeometry) just works.

@juliohm
Copy link
Member Author

juliohm commented May 20, 2025

Most JuliaGeo geometry packages can plot their objects in Makie now after a lot of complaints and confusion that they didn't.

Again, that is not true. You introduced these inappropriate dependencies in Shapefile.jl, GeoJSON.jl and ArchGDAL.jl. Packages like GeoParquet.jl and GeoArrow.jl are still clean.

If people complain that these geometries can't plot directly, you don't add dependencies. You recommend the installation of a dedicated package for plotting. The hacks here with GeoInterfaceMakie.jl + MakieCore.jl alongside a Makie.jl extension are far from an acceptable solution.

MakieCore is not particularly large but I totally agree making it a weak dep would be better.

It is not about size, it is about scope. Shapefile.jl is a package for reading/writing the shapefile format. Any dependency that does not contribute to this objective is unwelcome, and increases the potential of build failures in downstream applications.

Also, MakieCore.jl depends on GeometryBasics.jl, so you made Shapefile.jl depend on another set of geometries, which defeats the whole GeoInterface.jl story.

But we need to find a way for that to continue working with a weak dependency, not just removing all of the code and breaking user expectation that plot(shapefilegeometry) just works.

I hope @visr and other JuliaGeo maintainers will agree that your expectations should not have higher priority over good software engineering practices.

Weak dependencies are a good solution when a single package that defines types is placed in an extension for method specialization of functions that live in another package. Here you have a combo of MakieCore.jl + Makie.jl + GeoInterface.jl + GeoInterfaceMakie.jl that is just madness. And I won't even talk about invalidations...

If you want to plot these geometries defined in Shapefile.jl, create a monolithic "GeoMakie.jl" package that defines the plotting recipes for all the types you care about. These definitions can live in package extensions of GeoMakie.jl, not the other way around.

Copy link
Member

@visr visr left a comment

Choose a reason for hiding this comment

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

Let's not remove this feature until we have some consensus on what a possible better approach would be, which we can discuss in #138.

@juliohm
Copy link
Member Author

juliohm commented May 20, 2025

Let's not remove this feature until we have some consensus on what a possible better approach would be, which we can discuss in #138.

Why there needs to be a consensus before the merge? This PR solves a serious dependency issue in the Shapefile.jl package, which has nothing to do with plotting. The plotting story should be addressed outside of the Shapefile.jl repository.

@rafaqz
Copy link
Member

rafaqz commented May 20, 2025

Because this PR will break many people's workflows.

Please try to be constructive we can solve this for everyone, not just for you.

@juliohm
Copy link
Member Author

juliohm commented May 20, 2025

Because this PR will break many people's workflows.

You already broke other people's workflows when you unilaterally decided to add Makie-related deps to these packages. Can't you see the seriousness of this issue?

Please try to be constructive we can solve this for everyone, not just for you.

It is not just for me. You made a decision that was not widely discussed anywhere. I doubt the majority of users would vote in favor of adding Makie-deps to Shapefile.jl

@rafaqz
Copy link
Member

rafaqz commented May 20, 2025

Removing dependencies without losing plot recipes is solved in this PR, with minor change to the macro for dependent packages:
JuliaGeo/GeoInterface.jl#195

@asinghvi17
Copy link
Member

Closing in favor of #142

@visr visr deleted the rm-makie-plots-deps branch July 11, 2025 19:55
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.

Plotting dependencies are not ok

5 participants