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

Update the "Usage" example in the README #36

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Update the "Usage" example in the README #36

merged 2 commits into from
Jun 20, 2024

Conversation

joshday
Copy link
Collaborator

@joshday joshday commented Jun 18, 2024

Right after I merged my other PR I realized the README example was using the "test.dbf" file that I had deleted.

@visr
Copy link
Collaborator

visr commented Jun 18, 2024

I see you're using the path return value from write you added in #35. Is that something you've seen elsewhere? I think I used number of bytes first because that's what Base does. We could consider specifying it in JuliaGeo/GeoInterface.jl#134 as well.

@joshday
Copy link
Collaborator Author

joshday commented Jun 18, 2024

Hmm, I was thinking Base.write(path, x) did return the path, but it's JSON3.write and CSV.write that I borrowed it from.

I'm happy to change it back/follow whatever the consensus is. I strongly agree about keeping the interface consistent between packages.

I changed the example to work with either behavior.

@visr
Copy link
Collaborator

visr commented Jun 19, 2024

Right, good to know. Returning the path seems more useful than the number of bytes for this use case.

@joshday joshday merged commit 489b320 into main Jun 20, 2024
3 checks passed
@joshday joshday deleted the jd/readme branch June 20, 2024 00:05
@asinghvi17
Copy link
Contributor

From https://juliageo.org/GeometryOps.jl/dev/tutorials/creating_geometry#save-geometry it seems only Shapefile.jl returns the number of bytes, everything else returns the path.

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