gdalraster icon indicating copy to clipboard operation
gdalraster copied to clipboard

GDALVector: subtle issue with allowed constructor signature

Open mdsumner opened this issue 10 months ago • 2 comments

I noticed some sensitivity to the actual options when constructing:

f <- system.file("extdata/ynp_fires_1984_2022.gpkg", package = "gdalraster")
## not ok
new(GDALVector, f, "", read_only = TRUE, open_options = NULL)
#> Error: Not compatible with STRSXP: [type=NULL].

these all ok, note our attempt above is ok with 'open_options' being zero length character

new(GDALVector, f)
new(GDALVector, f, "")
new(GDALVector, f, "", read_only = TRUE)
new(GDALVector, f, "", read_only = TRUE, open_options = character())
new(GDALVector, f, "", read_only = TRUE, open_options = NULL, spatial_filter = "")
new(GDALVector, f, "", read_only = TRUE, open_options = NULL, spatial_filter = "", dialect = "OGRSQL")

It affected me in that I couldn't easily determine which args I could set, they seemed to be dependent on specific sets. I think my first attempt that led me down this road was this, which I guess means the args are not cleanly name-identified?

f <- system.file("extdata/ynp_fires_1984_2022.gpkg", package = "gdalraster")
new(GDALVector, f, "", read_only = TRUE, dialect = "")

Warning 6: open option '' is not formatted with the key=value format

C++ object of class GDALVector
 Driver : GeoPackage (GPKG)
 DSN    : /perm_storage/home/mdsumner/R/x86_64-pc-linux-gnu-library/4.4/gdalraster/extdata/ynp_fires_1984_2022.gpkg
 Layer  : mtbs_perims
 CRS    : NAD83 / Montana (EPSG:32100)
 Geom   : MULTIPOLYGON```

mdsumner avatar Feb 21 '25 07:02 mdsumner

This kind of testing and feedback is much appreciated as we get closer to a release with the vector API.

The above makes sense with what I see currently for the constructor logic, including where NULL for open_options works or doesn't. I'll go through and see what could be improved.

args are not cleanly name-identified?

That is possible. I'll check the Rcpp exposed class doc. I'm still very positive on exposed class but it's pretty close to C++.

Overall, this is one of the reasons I've been interested in a convenience function for constructing the objects that might be preferred by most users, e.g., gdal_open(). That way, input validation could be done in R code, argument defaults would be obvious, name-identified would be as expected, etc.

ctoney avatar Feb 21 '25 08:02 ctoney

Partially addressed in #654 but I want to leave this issue open for now. It's worth thinking through the constructors again, especially against your helpful analysis above, and look for possible improvements. May not come up with much but worthwhile to consider and will do that soon.

And then the gdal_open() idea which seems like it should be reasonably simple but I'm still mentally blocked on how to do it exactly.

ctoney avatar Mar 09 '25 08:03 ctoney