Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Merged
merged 7 commits into from
Jan 31, 2023

Conversation

jmaspons
Copy link
Collaborator

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

@jmaspons jmaspons force-pushed the fix_strange_keys branch 2 times, most recently from 1a3ea5a to be8045d Compare January 27, 2023 12:23
As per ?make.names, valid syntactic names only include letters, numbers and
the dot or underline characters and starts with a letter. For example, many
keys in OSM contain also «:» which get transformed to «.», then «name:ca»
becomes «name.ca»
Rename keys to key.n and throw a warning
# Conflicts:
#	R/get-osmdata.R
@jmaspons jmaspons marked this pull request as ready for review January 27, 2023 16:16
@jmaspons
Copy link
Collaborator Author

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

@mpadge
Copy link
Member

mpadge commented Jan 30, 2023

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.

R/get-osmdata.R Outdated Show resolved Hide resolved
R/get-osmdata.R Outdated Show resolved Hide resolved
@mpadge
Copy link
Member

mpadge commented Jan 30, 2023

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 and others added 2 commits January 30, 2023 18:24
Co-authored-by: mark padgham <mark.padgham@email.com>
Co-authored-by: mark padgham <mark.padgham@email.com>
@jmaspons
Copy link
Collaborator Author

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

@mpadge
Copy link
Member

mpadge commented Jan 31, 2023

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.

R/get-osmdata.R Outdated Show resolved Hide resolved
@mpadge
Copy link
Member

mpadge commented Jan 31, 2023

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

Co-authored-by: mark padgham <mark.padgham@email.com>
@jmaspons
Copy link
Collaborator Author

jmaspons commented Jan 31, 2023

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

Can try to add the NEWS update too

@mpadge
Copy link
Member

mpadge commented Jan 31, 2023

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

@mpadge mpadge merged commit de336b3 into ropensci:main Jan 31, 2023
@jmaspons jmaspons deleted the fix_strange_keys branch February 2, 2023 09:58
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] osmdata_* functions return features with changed tag names for tags with non-valid names
2 participants