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

Suggestion: remove/change curl::has_internet() from yfR #20

Closed
jtrecenti opened this issue Nov 11, 2022 · 4 comments
Closed

Suggestion: remove/change curl::has_internet() from yfR #20

jtrecenti opened this issue Nov 11, 2022 · 4 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@jtrecenti
Copy link

Hi! Thanks for this awesome package. We use it a lot in our classes at Insper.

We are facing some issues running yfR::yf_get() because curl::has_internet() returns FALSE, even though we have a stable internet connection here. Probably it is some proxy issue that we do not control.

A suggestion would be to remove this check (if the user does not have internet connection, the download will fail anyway) or add a check_internet= parameter having TRUE as default.

What do you think? Thanks!

@msperlin
Copy link
Collaborator

Hi Julio,

I think it is best to add a option as argument (check_internet). You would be surprised how many emails I get when people try to use the package without internet.

Btw, can you test pingr::is_online(), which seems to use another method. Let me know how it goes..

@msperlin msperlin self-assigned this Nov 11, 2022
@msperlin msperlin added bug Something isn't working good first issue Good for newcomers labels Nov 11, 2022
@jtrecenti
Copy link
Author

jtrecenti commented Nov 11, 2022

Thanks for the answer!

pingr::is_online() works as expected here at Insper.

pingr::is_online()
#> [1] TRUE

Created on 2022-11-11 with reprex v2.0.2

Do you prefer to

  1. change curl::has_internet() -> pingr::is_online() (more 📦 dependencies) or
  2. add a parameter (more parameters 😅)?

I'd be happy to make a PR

@jtrecenti
Copy link
Author

Another option is to use this test (used by testthat) instead of curl::has_internet()

> testthat::skip_if_offline
function (host = "r-project.org") 
{
    skip_on_cran()
    skip_if_not_installed("curl")
    has_internet <- !is.null(curl::nslookup(host, error = FALSE))
    if (!has_internet) {
        skip("offline")
    }
}

This one also works here. This way there's no need to add any new dependencies.

jtrecenti added a commit to jtrecenti/yfR that referenced this issue Nov 11, 2022
@msperlin
Copy link
Collaborator

Thanks for the answer!

pingr::is_online() works as expected here at Insper.

pingr::is_online()
#> [1] TRUE

Created on 2022-11-11 with reprex v2.0.2

Do you prefer to

  1. change curl::has_internet() -> pingr::is_online() (more package dependencies) or
  2. add a parameter (more parameters sweat_smile)?

I'd be happy to make a PR

Using pingr is alright, it is a minimal package.

Sure, fell free to send a PR. Otherwise I'll fix the code next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants