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.

Sign up for GitHub

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

fix read(io) to support Downloads.download of stdlib #90

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

fix read(io) to support Downloads.download of stdlib #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d3eaf4
fix read(io) to support Downloads.download of stdlib
likev Apr 28, 2024
36af205
Update README.md using Downloads instead of HTTP.jl
likev Apr 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@ julia> write(fc)


### HTTP access
To read JSON from a URL, use HTTP.jl
To read JSON from a URL, use Downloads
```julia

julia> using GeoJSON, HTTP
julia> using GeoJSON, Downloads

julia> resp = HTTP.get("https://path/to/file.json")
julia> io = Downloads.download("https://its-live-data.s3.amazonaws.com/datacubes/catalog_v02.json", IOBuffer())

julia> fc = GeoJSON.read(resp.body)
julia> fc = GeoJSON.read(io)
```
1 change: 1 addition & 0 deletions src/io.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Read GeoJSON to a GeoInterface.jl compatible object.
- `numbertype::DataType=Float32`: Use Float64 when the precision is required.
"""
function read(io; lazyfc=false, ndim=2, numbertype=Float32)
eof(io) && seekstart(io)
Copy link
Member

@asinghvi17 asinghvi17 Apr 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR!

This also applies to strings and Vector{UInt8} as written now, so the code should be abstracted out somewhere else. Perhaps a separate dispatch of read for ::IO objects? CI is failing for this reason.

Copy link
Member

Choose a reason for hiding this comment

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

Also needs some explanatory comments.

Copy link
Member

Choose a reason for hiding this comment

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

Is it common to do such a thing? I've never seen it. It seems more up to the caller that the IOBuffer should not be at the end.

Copy link
Author

Choose a reason for hiding this comment

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

Is it common to do such a thing? I've never seen it. It seems more up to the caller that the IOBuffer should not be at the end.

It's common to download GeoJSON data from web links when using GeoMakie

https://github.com/MakieOrg/GeoMakie.jl/blob/d10a28662457eecbcac0371484f0581c035eb82c/examples/german_lakes.jl#L6-L7

https://github.com/MakieOrg/GeoMakie.jl/blob/d10a28662457eecbcac0371484f0581c035eb82c/examples/specialized/graph_on_usa.jl#L9-L10

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree, I meant specifically the seekstart. That seems like it is not the responsibility of that function.

Copy link
Author

Choose a reason for hiding this comment

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

I agree that it is the responsibility of the caller to ensure the IO is in the usable status.
But check eof and seekstart will made GeoJSON.jl be user-friendly for beginners, especially for those who are unfamiliar with input/output streams.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I appreciate the effort for user-friendliness, but am afraid that then users will expect other functions reading streams do the same, and possibly end up more confused. To keep the example simple it is also fine to just use Downloads.download("https://its-live-data.s3.amazonaws.com/datacubes/catalog_v02.json") to get a temp file path.

Copy link
Author

Choose a reason for hiding this comment

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

maybe we can providing an seekstart bool argument and/or code example to facilitate this usage scenario.

if lazyfc
obj = JSON3.read(io, LazyFeatureCollection{ndim,numbertype})
else
Expand Down
Loading