search

ropensci / osmdata

R package for downloading OpenStreetMap data
https://docs.ropensci.org/osmdata
314 stars 45 forks source link

Fix key modifications for non-valid column names in osmdata_* functions #303

Closed jmaspons closed 1 year ago

jmaspons commented 1 year ago

FIX #301 and handle cases with clashes between OSM keys and id or metadata columns

jmaspons commented 1 year ago

Ready for review, @mpadge ! I'm not sure how silicate objects should handle clashes between tags and id or metadata keys

mpadge commented 1 year ago

I'm not sure how silicate objects should handle clashes between tags and id or metadata keys

That's a good question, but one that i'm happy to worry about later. As said, I'm pretty confident the only use of osmdata_sc() is by people interfacing that with dodgr, in which case metadata are not used anyway. I'll just keep that point in mind, and maybe address in the future if any issues arise.

mpadge commented 1 year ago

A few minor suggestions there. Let me know when they're done, and i'll happily merge. This is, as with all the others, a really good PR. Thank you so much! The fix_duplicated_columns function is something I hadn't even thought of, but is so obviously important, and excellently impelemted. Great stuff.

jmaspons commented 1 year ago

Thanks for the review, @mpadge ! I think it's ready now

mpadge commented 1 year ago

Thanks for the review, @mpadge ! I think it's ready now

Not quite. The tests are failing because of this:

devtools::load_all ()
#> ℹ Loading osmdata
#> Data (c) OpenStreetMap contributors, ODbL 1.0. https://www.openstreetmap.org/copyright
qry <- opq (bbox = c (-0.116, 51.516, -0.115, 51.517))
qry <- add_osm_feature (qry, key = "highway")
res <- osmdata_data_frame (qry)
#> Error in matrix(nrow = nrow(i), ncol = length(keys), dimnames = list(NULL, : non-numeric matrix extent

Created on 2023-01-31 with reprex v2.0.2 I'll just try to find out what's going wrong.

mpadge commented 1 year ago

Pretty sure this is the problem: https://github.com/ropensci/osmdata/pull/303/files#r1091737720 Hopefully tests will pass once you accept that suggestion :crossed_fingers:

jmaspons commented 1 year ago

indeed, that was the problem. All green now, @mpadge

Can try to add the NEWS update too

mpadge commented 1 year ago

Cool, thanks. Great that the tests now pass. Let me know when you've updated the news, and we'll merge away ...