Skip to content

Remove curl from suggests#5749

Merged
jangorecki merged 7 commits intomasterfrom
remove_curl_depends
Dec 6, 2023
Merged

Remove curl from suggests#5749
jangorecki merged 7 commits intomasterfrom
remove_curl_depends

Conversation

@ben-schwen
Copy link
Copy Markdown
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
Copy Markdown
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
@codecov
Copy link
Copy Markdown

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

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

@ben-schwen
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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