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

Fix queries with multiple features and more than one osm_types #318

Merged
merged 3 commits into from
Mar 7, 2023

Conversation

jmaspons
Copy link
Collaborator

@jmaspons jmaspons commented Mar 5, 2023

Example of the error for osm_types and features with non-multiple lengths

q <- opq("Catalunya") |> 
    add_osm_features(features = c("\"amenity\"=\"school\"",
                                  "\"amenity\"=\"kindergarten\"",
                                  "\"amenity\"=\"music_school\"",
                                  "\"amenity\"=\"language_school\"",
                                  "\"amenity\"=\"dancing_school\""))
opq_string(q)
#> Error in sprintf("  %s %s (%s);\n", opq$osm_types, features, opq$bbox) :
#>  arguments cannot be recycled to the same length

For osm_types and features with multiple lengths, every feature gets only one osm_type instead of all osm_types

q <- opq("Catalunya") |> 
    add_osm_features(features = c("\"amenity\"=\"school\"",
                              "\"amenity\"=\"kindergarten\"",
                              "\"amenity\"=\"music_school\""))
cat(opq_string(q))
#> [out:xml][timeout:25];
#> (
#>   node ["amenity"="school"] (40.5229822,0.1594133,42.8615226,3.3222508);
#>   way ["amenity"="kindergarten"] (40.5229822,0.1594133,42.8615226,3.3222508);
#>   relation ["amenity"="music_school"] (40.5229822,0.1594133,42.8615226,3.3222508);
#> );
#> (._;>;);
#> out body;

Error in sprintf("  %s %s (%s);\n", opq$osm_types, features, opq$bbox) :
  arguments cannot be recycled to the same length
@jmaspons jmaspons requested a review from mpadge March 5, 2023 23:18
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.

That's great. Could you maybe just add a test to illustrate the effect that this has? A suggestion would be in the osm_types test after this line:

expect_true ("nwr" == q1$osm_types)

something like this (with the actual getbb() call from opq() switched off, of course):

features <- c(
    "\"amenity\"=\"school\"",
    "\"amenity\"=\"kindergarten\"",
    "\"amenity\"=\"music_school\"",
    "\"amenity\"=\"language_school\"",
    "\"amenity\"=\"dancing_school\""
)
q <- opq("Catalunya") |> 
    add_osm_features(features = features)
s <- opq_string(q)

n_fts <- length (features)
n_fts_in_query <- length (gregexpr ("amenity", s) [[1]])
# Query should have that number repeated for each of (node, way, relation):
expect_equal (n_fts_in_query, n_fts * 3L)

@jmaspons
Copy link
Collaborator Author

jmaspons commented Mar 7, 2023

Tests added and NEWS updated

@jmaspons jmaspons merged commit 31b9a6e into ropensci:main Mar 7, 2023
@jmaspons jmaspons deleted the fix_osm_type-features branch March 7, 2023 17:04
@mpadge
Copy link
Member

mpadge commented Mar 7, 2023

Great, thanks!

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