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

Add parameters from oqp to opq_osm_id #320

Merged
merged 3 commits into from
Mar 16, 2023
Merged

Conversation

jmaspons
Copy link
Collaborator

No description provided.

@jmaspons jmaspons requested a review from mpadge March 14, 2023 23:19
Copy link
Member

@mpadge mpadge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for thinking of that. This is a really cool expansion, but still needs a bit of work to reconciile the kinds of behaviour illustrated in these couple of reprexes.

devtools::load_all (".", export_all = TRUE)
#> ℹ Loading osmdata
#> Data (c) OpenStreetMap contributors, ODbL 1.0. https://www.openstreetmap.org/copyright
id <- 142107768 # Boundary of Bois de Vincennes, Paris: way
x0 <- opq_osm_id (type = "way", id = id) |>
    opq_string () |>
    osmdata_sf ()
n0 <- nrow (x0$osm_points)

dt0 <- "2013-01-01T00:00:00Z"
dt2 <- "2023-03-15T00:00:00Z"

# Expect fewer points in 2013 than 2023:
x1 <- opq_osm_id (type = "way", id = id, datetime = dt0) |>
    opq_string () |>
    osmdata_sf ()
n1 <- nrow (x1$osm_points)
n1 < n0
#> [1] TRUE

# Uninformative error message: should direct people to use
# `osmdata_data_frame()`:
x <- opq_osm_id (type = "way", id = id, datetime = dt0, datetime2 = dt2, adiff = TRUE) |>
    opq_string () |>
    osmdata_sf ()
#> Error in check_not_implemented_queries(obj): adiff queries not yet implemented.

# This is then okay:
x <- opq_osm_id (type = "way", id = id, datetime = dt0, datetime2 = dt2, adiff = TRUE) |>
    opq_string () |>
    osmdata_data_frame ()
nrow (x) < n1
#> [1] TRUE

Created on 2023-03-15 with reprex v2.0.2

Plus then these illustrations of error and warning messages that need to be improved:

devtools::load_all (".", export_all = TRUE)
#> ℹ Loading osmdata
#> Data (c) OpenStreetMap contributors, ODbL 1.0. https://www.openstreetmap.org/copyright
id <- 7444 # Boundary of Paris, France: relation

# This error message is uninformative, and should direct people to use
# 'osmdata_data_frame()':
x <- opq_osm_id (type = "relation", id = id, out = "tags") |>
    opq_string () |>
    osmdata_sf ()
#> Error in check_not_implemented_queries(obj): Queries returning no geometries 
#> (out tags/ids) not accepted. Use queries with `out="body"` or `out="skel"` instead.

# This then works as expected
x <- opq_osm_id (type = "relation", id = id, out = "tags") |>
    opq_string () |>
    osmdata_data_frame ()

# Same here: the warning (okay in this case), should also direct people to use
# `osmdata_data_frame()`:
x <- opq_osm_id (type = "relation", id = id, out = "meta") |>
    opq_string () |>
    osmdata_sf ()
#> Warning in check_not_implemented_queries(obj): `out meta` queries not yet
#> implemented. Metadata fields will be missing.

# Which again works as expected:
x <- opq_osm_id (type = "relation", id = id, out = "meta") |>
    opq_string () |>
    osmdata_data_frame ()

Created on 2023-03-15 with reprex v2.0.2

@jmaspons
Copy link
Collaborator Author

Hi and thanks for the feedback, @mpadge!

My assumption for the text of the messages is that people using osmdata_sp/sf/sc are really interested in the spatial data, thus the message points to changes in the query. Perhaps we can add the tip to use osmdata_xml/data_frame, as explained in ?opq details. Is that OK?

@mpadge
Copy link
Member

mpadge commented Mar 15, 2023

Hi and thanks for the feedback, @mpadge!

My assumption for the text of the messages is that people using osmdata_sp/sf/sc are really interested in the spatial data, thus the message points to changes in the query. Perhaps we can add the tip to use osmdata_xml/data_frame, as explained in ?opq details. Is that OK?

Yeah, sorry, I should have explained better, but that was exactly what I meant. Just adding some extra information to current errors/warnings.

@jmaspons
Copy link
Collaborator Author

I improved the messages, @mpadge. Ready to merge now?

@mpadge mpadge self-requested a review March 16, 2023 09:31
Copy link
Member

@mpadge mpadge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, that's much better, thanks

@mpadge mpadge merged commit ca26b9f into ropensci:main Mar 16, 2023
@jmaspons jmaspons deleted the opq_osm_id branch March 16, 2023 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants