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

Issues of depending both on httr and httr2 #17

Closed
salvafern opened this issue Feb 13, 2023 · 5 comments
Closed

Issues of depending both on httr and httr2 #17

salvafern opened this issue Feb 13, 2023 · 5 comments
Assignees

Comments

@salvafern
Copy link
Collaborator

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:

  • Gazetteer:
  • Data products:
    • API endpoint: https://geo.vliz.be/geoserver
    • Imports: ows4R, which depends in httr and not in httr2
    • Suggests: leaflet + leaflet.extras2 for WMS visualization (moved to suggests and added check to avoid package failure in case of missing library leaflet)
    • Testing: httptest to mock up HTTP responses

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 fa2acce

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
Copy link
Member

maelle commented Feb 14, 2023

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

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 salvafern self-assigned this Feb 14, 2023
@salvafern
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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 salvafern changed the title Downgrade httr2 -> httr OR split the package in 2 Issues of depending both on httr and httr2 Feb 21, 2023
@salvafern
Copy link
Collaborator Author

Asked in eblondel/ows4R#103

@salvafern
Copy link
Collaborator Author

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.

@salvafern salvafern closed this as not planned Won't fix, can't repro, duplicate, stale Mar 7, 2023
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

No branches or pull requests

2 participants