search

ropensci / osmdata

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

Add lat and lon columns for `out * center;` queries in `osmdata_data_frame` #316

Closed jmaspons closed 1 year ago

jmaspons commented 1 year ago

For calls to osmdata_data_frame that do not include the query and the results only contain nodes, there is no way to distinguish if center lat & lon columns have to be added or not. The current implementation adds osm_center_lat/lon in these cases

Fix #315

jmaspons commented 1 year ago

Thanks for the review, @mpadge. Everything addressed, so I'm going to merge

mpadge commented 1 year ago

Thanks for the review, @mpadge. Everything addressed, so I'm going to merge

@jmaspons Please don't do so until I have another look through and re-review this. Can you please request another review from me? Thanks

jmaspons commented 1 year ago

One thing i'm not sure about - due to me not understanding it fully smirk - is these lines (in "get-osmdata-df.R"::xml_to_df()):

    if (any (missing_center)) { # not a "out * center;" query
        center <- lapply (center, function (x) {
            x[, c ("osm_center_lat", "osm_center_lon")] <- NULL
            x
        })
    }

They now don't do anything at all, right? And so can be removed, right?

This part remove osmcenter columns if in doc exist objects types without center data (checked 3 lines above these lines). The porpoise is to identify queries which should not return center columns (no `out center;). I do like this to avoid passing the query to the function. The problematic case is for results that only contain nodes, because nodes always contain lat & lon (relations and ways only when query hasout * center;`).

Removing the columns like this instead of center <- data.frame() keeps nrow which is necessary as explained above