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

Get a clear error message if settings are empty #154

Open
florianm opened this issue Feb 10, 2020 · 3 comments
Open

Get a clear error message if settings are empty #154

florianm opened this issue Feb 10, 2020 · 3 comments

Comments

@florianm
Copy link
Contributor

florianm commented Feb 10, 2020

If ckanr is not set up with valid credentials, it is all too easy to get stuck, typically when libcurl complains about an empty protocol not being supported. The error chain goes like

  • env vars CKAN_DEFAULT_URL and friends are empty (silent)
  • ckanr::get_default_xxx() returns these empty values (silent)
  • crul calls curl with empty url and/or credentials (silent)
  • curl calls libcurl (silent)
  • libcurl receives empty url, parses it into parts, falls over empty protocol (should be http or https), and finally raises error message "not supported"

I contributed to the ckanr settings in 2015 and have been using ckanr in production since then, but I still get bitten by missing settings and would end up less often in the Corner of ShameTM if ckanr settings / ckanr setup could warn loudly and clearly so I'd remember to run ckanr_setup() with valid settings.

A setup that yells clear error messages is e.g. here https://dbca-wa.github.io/ruODK/reference/ru_settings.html which initially was stolen from ckanr settings but improved by rOpenSci reviewer comments.

Rough idea:

  • Prepend each web request with a pat down for missing credentials, cf. ref, code, example. I assume that anywhere outside tests each function wants to call a real CKAN instance.
  • The ckanr getters could issue a friendly warning message if the respective value comes back empty. I can't think of any use case where empty defaults would be desired.

@sharlagelfand @sckott what do you reckon?

@sckott
Copy link
Contributor

sckott commented Feb 12, 2020

totally agree we should fail early and let users know exactly what to do.

a helper fxn at the top of each function to check settings would be good. in ckanr, url is the only required setting, and key is optional

@florianm
Copy link
Contributor Author

Would you say that testing whether the url is empty might be enough of a smoke test?

Would you feel that the default getters should read from env vars? Example https://github.com/dbca-wa/ruODK/blob/master/R/ru_setup.R#L288
That way, setting the env vars in .Renviron would automatically setup ckanr. Long form https://dbca-wa.github.io/ruODK/articles/setup.html#permanent-defaults-environment-variables

@sckott
Copy link
Contributor

sckott commented Feb 13, 2020

Yeah, if url is empty, or length zero then the fxn should fail and tell the user.

default getters should read from env vars?

they do already do that https://github.com/ropensci/ckanr/blob/master/R/ckanr_settings.R#L154 but they don't warn when it's missing though i guess

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