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

higher level StringList option alternative like Dict #194

Open
visr opened this issue May 17, 2021 · 6 comments
Open

higher level StringList option alternative like Dict #194

visr opened this issue May 17, 2021 · 6 comments
Assignees
Milestone

Comments

@visr
Copy link
Collaborator

visr commented May 17, 2021

At several points in GDAL a StringList is expected for options. I somehow always forget what it should look like. Do you think it would be a good idea to accept Dicts or some other key/value datatype there instead?

These kind of options don't feel very julian: options = ["GEOM_POSSIBLE_NAMES=point,linestring", "KEEP_GEOM_COLUMNS=NO"], from https://yeesian.com/ArchGDAL.jl/latest/tables/.

Not sure if we discussed this before, though I see an old comment here about taking in Dicts in higher level functions:
JuliaGeo/GDAL.jl#3 (comment)

Provide higher-level julia functions (auto-generated or handwrapped) that take-in and emit julian types (Dict/Vector/UTF8String/etc), that we'll have to write documentation for.

This kind of came up in evetion/GeoDataFrames.jl#14.

Looking at how rasterio does creation options for instance, there are a lot of these for GeoTIFF:
https://gdal.org/drivers/raster/gtiff.html#creation-options
And can be passed as keyword arguments like this:
https://rasterio.readthedocs.io/en/latest/topics/image_options.html#creation-options

This goes a bit further than Dict{String, String} since it lowercases the keys and maps "YES" to true etc, so we'd have to think if this can be done automatically to prevent the extra maintenance of these option mappings.

@yeesian
Copy link
Owner

yeesian commented May 20, 2021

Do you think it would be a good idea to accept Dicts or some other key/value datatype there instead?

Definitely.

As a proposal: I'm curious what you think of having NamedTuples or keyword args (c.f. #187)?

@yeesian yeesian added this to the v1.0 milestone May 20, 2021
@yeesian
Copy link
Owner

yeesian commented May 20, 2021

so we'd have to think if this can be done automatically to prevent the extra maintenance of these option mappings.

Thanks for the extra thoughtfulness here and yeah, at first glance,I think the main requirements I can think of so far are:

  1. The names are correspondent to the GDAL option names.
    • If the GDAL option names are always going to be uppercase, then we can support case-insensitivity in the keyword names (by casting everything to uppercase before passing it to GDAL.
  2. The values can be marshaled to string in a straightforward fashion.
    • E.g. we can initially support e.g. String, Real, Bool and Vector{String} values and expand the set of types/possibilities over time based on what we learn?
  3. The documentation for the options can be easily updated
    • I think it might be difficult for us to auto-update the (handcrafted) docstrings here with the official documentation, so we could provide examples for a limited subset of options that are known to be stable over time, and point to the official (or GDAL.jl) documentation for other possibilities.

@rafaqz
Copy link
Collaborator

rafaqz commented May 20, 2021

This would be great, I've had a similar response to @visr a few times trying to work out how to pass options.

Keyword arguments feel the most Julian, but a Dict/NamedTuple is also a lot better than Vector{String}. Allowing lowercase would also feel more Julian than all caps.

Also consider allowing Symbol instead of String?

So commands could be something like:

...; geom_possible_names=(:point, :linestring), keep_geom_columns=false)

@visr
Copy link
Collaborator Author

visr commented May 20, 2021

Yes I think keyword arguments would probably be the nicest looking. One thing I wonder about for keywords, does it ever become problematic, them mixing with the existing keywords?

Right now we have:

read(filename; flags=OF_ReadOnly, alloweddrivers, options, siblingfiles)

Do we then do this?

read(filename; flags=OF_ReadOnly, alloweddrivers, siblingfiles, options...)

Such that the options still end up together in a single variable, and we don't mix them up with the other keyword arguments?

@yeesian
Copy link
Owner

yeesian commented May 28, 2021

Echoing #194 (comment), yeah I think all non-option keywords will have to be explicitly spelt out for those functions that accept options as kwargs.... There can also be a second version which is correspondent to the original function, taking in the options as a NamedTuple argument?

@rafaqz
Copy link
Collaborator

rafaqz commented May 28, 2021

Yeah it's probably possible to allow both.

@yeesian yeesian self-assigned this Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants