ropensci / mregions2

Access the Marine Regions Gazetteer and the Marine Regions Data Products in R. Maintained by @salvafern.
https://docs.ropensci.org/mregions2/
Other
5 stars 2 forks source link

Issues of depending both on httr and httr2 #17

Closed salvafern closed 1 year ago

salvafern commented 1 year ago

As mentioned before, there are two well distinguished parts in this package with different functions:

  1. Those reading the Gazetteer REST API + RDF. e.g. gaz_search("Belgian Part of the North Sea)
  2. Those focused in visualizing and loading the data products via OGC. e.g. mrp_get("eez_boundaries")

Some details:

This brings a problem: we used the latest httr2 for the gazetteer, but due to ows4R we also need to depend on httr

And because we depend both on httr and httr2, we need to use both httptest and httptest2 to mock up calls. This brought me some headaches as the two packages tend to overlap, but it is solved by cleaning after the use of each test package. E.g. detach and set requesters / or redactors to NULL in each test. See https://github.com/lifewatch/mregions2/commit/fa2acce90e314821d391fe90dc3dbdcd14a4f6d2

I see three options that make sense here. Either:

  1. Downgrade the whole package to use httr instead of httr2, or
  2. Splitting the package in 2, one for the gazetteer API and one for the data products depending on ows4R
  3. Keep it as is, but this may make maintenance harder, plus increase dependencies unnecessarily.

As we are planning to submit this to ROpenSci, @maelle Could I ask you to quickly give your opinion on what's the best way forward in this case, so I can do the necessary changes before submitting? Thanks a lot in advance 😊

maelle commented 1 year ago

This is my personal opinion, not necessary the opinion of the whole editorial board :wink:

To me if you have a clear separation of functionality and dependencies between the potential two packages, a split sounds good. Reasons I think a split sounds good: easier for you to maintain, easier for an external contributor (or a reviewer) to get acquainted with the codebase.

With maybe an umbrella package (like taxize / tidyverse / devtools) that'd call the other two packages?

salvafern commented 1 year ago

Thanks a lot @maelle ! We will most likely do the split then and add an umbrella package as you suggested. I hope we can submit to rOpenSci within the next weeks.

salvafern commented 1 year ago

Discussed internally with part of the MR Team (@LennertSchepers & @brittlnv). We won't split the package for now. First we will make sure if there are any plans on ows4R to upgrade to httr2 and if not we may write our own WFS calls and retrieve via httr2.

This would have even some advantages as e.g. we can download to disk directly and use a output format more efficient than GML.

salvafern commented 1 year ago

Asked in https://github.com/eblondel/ows4R/discussions/103

salvafern commented 1 year ago

We have decided internally to leave it as is for now as the tests are working (see #16). We will revisit this during the rOpenSci review, maybe there will be new ideas raised.