update `add_osm_features()` per #277 + minor bugfix for `available_tags()` + possible `opq_osm_id()` feature addition

[elipousson]

This pull request updates add_osm_features() per #277. I was initially trying to support the option of passing a custom key_pre and bind vector along with the named vector or list of features. To do this I created a helper function, set_bind_key_pre() to share handling of key_exact and value_exact w/ add_osm_feature(). Ultimately, I decided a less granular approach that supported the same key_exact and value_exact parameters as add_osm_feature() was sufficient but left the structure in place if needed in the future for more customizable queries. I also added several other utility functions that I thought helped make the code more readable but can incorporate back into the main functions if preferred.

I also included a small bug fix for available_tags(). Currently, if there is a red edit link on the OSM Wiki for a key, the returned list of tags includes an invalid tag, e.g. available_tags("building") returns the tag "library&action=edit&redlink=1". I added a line with a gsub() function to remove the ampersand and everything after.

Lastly, included one additional feature: adding the option to opq_osm_id() to pass id with a prefixed type, e.g. "relation/11158003". I've added a similar feature to a wrapper function I've built for osmdata because it is easy to copy the prefixed id out of an OSM url. Obviously, I can remove this change if it is unwanted.

[elipousson]

Whoops – just spotted that I left a parameter definition for bind with add_osm_features() after I removed it as an option. I moved that to the unexported set_bind_key_pre() function and cleaned up the example formatting to match the existing style. The tests that require with_mock_dir() are failing but everything else should be working.

[mpadge]

Thanks @elipousson, once you've address the format issue mentioned above, I'll be better able to see what's going on here, and will merge asap after.

[mpadge]

Thanks @elipousson. Just a couple of small remaining TODOs, mostly ensuring test coverage of new error conditions, all in opq.R:

Please add tests to cover these error conditions (which are helpfully flagged in the PR by codecov these days :smile:):

• [ ] L305: "Bind must be length 1 or the same length as features"
• [ ] L313: "key_pre must be length 1 or the same length as features"
• [ ] L365: "bind must only include ..."
• [ ] L369: "key_pre must only include ..."

If possible, please try to trigger the errors through direct calls to add_osm_features(). The test approach in this package is to try as much as possible to avoid testing by simply calling the internal functions directly (such as check_bind_pre_key), and instead to demonstrate how typical calls to the exported functions trigger those errors. Other than those tests, everything looks great. It'll also need one final tweak: Please add yourself as "ctb" in the DESCRIPTION file. Thanks :rocket:

[elipousson]

Thanks, @mpadge!

The only issue with writing those tests is that I dropped the option to pass bind and key_pre directly from add_osm_features() since I was unsure how to prioritize parameters in case of conflict between the key_exact and value_exact parameters and whether there was a use case for the more complex queries passing the bind and key_pre values manually would allow. I had left the error checking code in place just in case there was an interest in bringing back that option later but, for code coverage, it may be easiest just to remove those checks entirely.

Let me know if you'd like to see bind and key_pre accessible via add_osm_features() but, if not, I'll simiplify the code and make sure everything has a corresponding test to go with it.

[mpadge]

Ah, i see. Then i think it would be good to leave the current code including the stop conditions listed above, and then write an additional, separate test with a note in the test file that these conditions are currently not possible, and can only be triggered by direct calls to the internal functions. Then just note that they may be exposed later if we decide that directly exposing the bind and key_pre parameters of set_bind_ikey_pre is useful. Thanks, and sorry for the extensive back and forth here :smile:

[mpadge]

Thanks so much @elipousson - this is a great improvement! I'll update the news.m in a moment with your contributions.

[elipousson]

Awesome. Very glad to contribute!