-
Notifications
You must be signed in to change notification settings - Fork 59
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
Allow to convert to other types, including auto #125
Conversation
Codecov Report
@@ Coverage Diff @@
## master #125 +/- ##
==========================================
+ Coverage 63.64% 63.81% +0.17%
==========================================
Files 29 29
Lines 1433 1440 +7
==========================================
+ Hits 912 919 +7
Misses 521 521
Continue to review full report at Codecov.
|
writeOGR appears to always write FeatureCollections, so that is what we will always get for Spatial objects
Ok, I lied above. sp objects are always forced to FeatureCollections because that's what So currently, for To return to the previous behaviour for |
thanks will have a look, sorry for delay was on away vacay on weekend to be clear, do you think it's good to go now, assuming i'm on board with changes? |
Definitely no need to apologize - it was the weekend! I think something needs to be done with |
thanks for clarification |
@ateucher hmm, for sp classes, looks like you have a pattern like (beware, pseudocode) geojson_json.SpatialPolygons <- function(other_params, type='FeatureCollection') {
geoclass(other_params, type = 'FeatureCollection')
} so For data.frame/numeric/list, does this look good? # define a helper function
check_type <- function(x) {
types <- c('FeatureCollection', 'GeometryCollection')
if (!x %in% types) stop("'type' must be one of: ", paste0(types, collapse=", "))
}
# use at the top of each function data.frame/numeric/list methods
check_type(type) geojson_json(c(-99.74,32.45), type="foobar")
#> Error in check_type(type) :
#> 'type' must be one of: FeatureCollection, GeometryCollection |
@sckott that's just what I was thinking as well. For sp classes, yes the hard-coded |
If you have the time, I'm happy if you merge this then add the |
Okay, will do the merge then add the type checker for those methods. submitting |
Awesome, thanks so much @sckott |
This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue. |
Not ready for merge.
Right now we currently only support FeatureCollection and GeometryCollection, but there's no reason we couldn't expand that. I started on this by modifying the
geoclass
function to accept all types, including'auto'
using the newgeojson::to_geojson
function.This looks like it works well for
sf
andSpatial
objects, but to implement for other classes will need more work to accommodate the different types.num_to_geo_list
andlist_to_geo_list
will need to be modified at the very least. Or we could (temporarily?) restrict thegeojson_json
methods forlist
,data.frame
, andnumeric
to deal only support FeatureCollection and GeometryCollection as they do now, and kick this one down the road...