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

Remove curl from suggests #5749

Merged
merged 7 commits into from
Dec 6, 2023
Merged

Remove curl from suggests #5749

merged 7 commits into from
Dec 6, 2023

Conversation

ben-schwen
Copy link
Member

@ben-schwen ben-schwen commented Nov 22, 2023

Closes #1686
Towards #5745

  • fix
  • tests (had to tests for downloading from secure urls before besides)
  • news
  • translations (done in release cycle)

Regarding translations: (@MichaelChirico)
I've never noticed these so far, but are they adapted with every release?

@MichaelChirico
Copy link
Member

I've never noticed these so far, but are they adapted with every release?

Would help if you're more specific, but if you're talking about anything in the po/ or inst/po folders, those are updated as part of the (normal) release process -- no need to touch in this PR.

This was referenced Nov 25, 2023
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5e8ca96) 97.46% compared to head (2896ebc) 97.46%.

❗ Current head 2896ebc differs from pull request most recent head 78894fd. Consider uploading reports for the commit 78894fd to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5749   +/-   ##
=======================================
  Coverage   97.46%   97.46%           
=======================================
  Files          80       80           
  Lines       14814    14814           
=======================================
  Hits        14439    14439           
  Misses        375      375           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jangorecki
Copy link
Member

AFAIU all what is needed still here is to escape test of that functionality for R 3.1.0.

@ben-schwen
Copy link
Member Author

We have never added a test to really download from an url when enabling this, so should we add one now? Must ensure then that the file is up and reachable.

@jangorecki
Copy link
Member

easy way to figure one which test needs to be escaped is to just leave the error for all versions of R, run test, and then you will see which tests failed, and those escape for R < 3.2.2

@jangorecki
Copy link
Member

We have never added a test to really download from an url when enabling this, so should we add one now? Must ensure then that the file is up and reachable.

to not introduce dependency on the external url, which may not always be available, to pass/fail test, I would prefer not to add test for that, if there is none already.

@ben-schwen
Copy link
Member Author

ben-schwen commented Dec 6, 2023

Regarding translations, we have this in our .pot:

data.table/po/R-data.table.pot

Lines 1255 to 1256 in 5e8ca96

msgid "Input URL requires https:// connection for which fread() requires 'curl' package which cannot be found. Please install 'curl' using 'install.packages('curl')'."
msgstr ""

Does this need to be addressed now or when the translations are updated?

@jangorecki
Copy link
Member

No need to worry about translations during dev cycle. It is part of release cycle.

@ben-schwen ben-schwen marked this pull request as ready for review December 6, 2023 15:05
@jangorecki jangorecki merged commit 63300ff into master Dec 6, 2023
3 checks passed
@jangorecki jangorecki deleted the remove_curl_depends branch December 6, 2023 15:09
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

Successfully merging this pull request may close these issues.

[bug] fread does not work with https url behind a proxy
3 participants