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

remove convert methods #84

Merged
merged 2 commits into from
Apr 29, 2023
Merged

remove convert methods #84

merged 2 commits into from
Apr 29, 2023

Conversation

rafaqz
Copy link
Member

@rafaqz rafaqz commented Jan 28, 2023

I went to start rewriting convert for GeoInterface but it just doesn't make sense to convert to these objects. We can write any geointerface geometries to shapefile anyway now, and it's a useless format for intermediate use. So its better to not maintain this code.

So this PR just removes all the convert methods.

@rafaqz rafaqz requested a review from visr January 28, 2023 18:40
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.

I'm fine with this. I hope this code is hardly used. It is still breaking though. Does it make sense to bring Shapefile.jl to 1.0? We could have a look if there are other possible things that we still want to change.

@rafaqz
Copy link
Member Author

rafaqz commented Feb 1, 2023

I doubt anyone has ever used it... we only got write recently so there was no reason to convert, and you don't need to convert to write anyway! It was just a dumb thing I wrote trying to implement all of GeoInterface here.

But yes a 1.0 could be nice, although we could probably fix a few things before then?

I never loved the name of Shapefile.Handle...

@visr
Copy link
Member

visr commented Feb 1, 2023

Ok then perhaps we can just merge this as non breaking.

Indeed I'm not a fan of Handle either.

@rafaqz rafaqz merged commit cf92205 into main Apr 29, 2023
@rafaqz rafaqz deleted the remove_convert branch April 29, 2023 21:24
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