Depend on R >= 4.1 after dropping magrittr dependence

[jmaspons]

Simplify examples assuming R >= 4.1 + fixes from examplesIf branch

FIX #358
DROP #369

[mpadge]

thanks @jmaspons. I've added suggestions reverting the not-run examples back to the original text, rather than opq(bb). I said in one comment that I think this is how most people would use this functionality, but I also think it demonstrates the important point that opq() can equally accept either bounding coordinates or place names. If that's all okay with you, could you please accept all suggestions and then update docs. Thanks!

[mpadge]

Sorry @jmaspons, but another request for change: All of the opq("place name") calls need to be in \dontrun, because they call the nominatim API to get the bounding box. The only ones that can be run are opq(bb) with pre-defined numerical bb values.

[jmaspons]

Ready for a new round, @mpadge

For ropensci-review-tools/pkgcheck#246 (comment), what's the problem running the tests? Is just the long runtime? Errors if !has_internet()?

[mpadge]

Ready for a new round, @mpadge

Thanks so much for all your work. The only minor suggestions I've now added are removing all of the # bounding box of <place> comments. I only put those there to explain what the (x0, y0, x1, y1) versions were, but they're now redundant as all place names are given immediately after. I'll leave you to accept those so you can re-generate the docs on your branch. Then we should be good to go! I'll approve now anyway, and once you've done that, please go ahead and merge. Thanks!! 🚀

For ropensci-review-tools/pkgcheck#246 (comment), what's the problem running the tests? Is just the long runtime? Errors if !has_internet()?

I was just referring to the fact that in our case we have to have examples witth \dontrun.

[jmaspons]

I was just referring to the fact that in our case we have to have examples witth \dontrun.

But why? It's because it would take too much time to check the package? You don't want to bother overpass servers? Instabilities due to relaying on overpass servers? I tried to check all the code in examples but the donttest container from r-hub seems to skip them anyway r-hub/containers#94

By the way, valgrind seems to find problems. I don't know if they are relevant or fixable

[mpadge]

I was just referring to the fact that in our case we have to have examples witth \dontrun.

But why? It's because it would take too much time to check the package? You don't want to bother overpass servers? Instabilities due to relaying on overpass servers? I tried to check all the code in examples but the donttest container from r-hub seems to skip them anyway r-hub/containers#94

Oh sorry, no just because opq("place name") calls the Nominatim server, and so will fail CRAN's requirement for "graceful failure" if the server is not reachable (which happens often enough). So they can't be run under any @examplesIf condition (except maybe examplesIf ami::on_cran(), but I doubt they'd take kindly to that 😏). Everything is tested anyway, so it's fine, but I do think this is a clear case where simple \dontrun{} statements like we have make for the clearest code in our examples.

By the way, valgrind seems to find problems. I don't know if they are relevant or fixable

Oh thanks, always good to check those, but they're all caused by stuff outside our package. The key there is they're only "possibly lost" at the very end. That's generally okay, while "definitely lost" signal genuine problems. Our own https://github.com/ropensci/osmdata/blob/main/tests/valgrind-test.R is run each time with tests anyway, and that should be considered the definitive checker of valgrind errors. (And in case you're unsure, valgrind checks for memory leaks in compiled code.)

[jmaspons]

Ok! It seems the valgrind test is not ran anymore and tests/valgrind-script.R was removed, but now I see that the logs from r-hub points to libudunits2 and libexpat.

[mpadge]

No, valgrint-test.R is still there. If you click on a "test-coverage" run, then open the "Test Coverage" step, at the top somewhere you should see this:

Running specific tests for package ‘osmdata’
  Running ‘testthat.R’
  Running ‘timing-benchmark.R’
  Running ‘valgrind-test.R'

And the script should do nothing, but it will error if anything goes wrong. Just so that you know that that script is actually important, and is still used in every test run

[jmaspons]

I was referring to valgrind-script.R that was removed and referenced in valgrind-test.R. Searching valgrind in https://github.com/ropensci/osmdata/actions/runs/15874044044/job/44757270849 finds nothing.

Also, running vg_check() from valgrind-test.R throws

Error in `<current-expression>` : error in running command
Called from: system2(command = "R", args = c("-d \"valgrind --tool=memcheck --leak-check=full\"", 
    "-f valgrind-script.R"), stdout = TRUE, stderr = TRUE)

Just to learn new things, it's fine as it is.

[mpadge]

Oh yeah, you're right! The function is just a definition. The script was the bit that actually ran it. I'll try to get it going again. Thanks! (And twalt ervor is probably just because you don't have valgrind installed.)