Skip to content
Please note that GitHub no longer supports your web browser.

We recommend upgrading to the latest Google Chrome or Firefox.

Learn more
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

isfiletext() checks whether a file is text or binary… #239

Merged
merged 8 commits into from Dec 21, 2019

Conversation

@bokov
Copy link
Contributor

bokov commented Oct 8, 2019

…in a cross-platform manner, closing #236. This can be useful when a file extension is missing or ambiguous

Please ensure the following before submitting a PR:

  • if suggesting code changes or improvements, open an issue first
  • for all but trivial changes (e.g., typo fixes), add your name to DESCRIPTION
  • for all but trivial changes (e.g., typo fixes), documentation your change in NEWS.md with a parenthetical reference to the issue number being addressed
  • if changing documentation, edit files in /R not /man and run devtools::document() to update documentation
  • add code or new test files to /tests for any new functionality or bug fix
  • make sure R CMD check runs without error before submitting the PR
…orm matter, completing ticket #236. This can be useful when a file extension is missing or ambiguous
@bokov

This comment has been minimized.

Copy link
Contributor Author

bokov commented Oct 8, 2019

There are failures but they caused by unavailability of libraries, not the new code.

MacOS only on Travis, the Linux builds work:
trying URL 'https://cloud.r-project.org/src/contrib/data.table_1.12.4.tar.gz'

Content type 'application/x-gzip' length 5010321 bytes (4.8 MB)

==================================================

downloaded 4.8 MB

  • installing source package ‘data.table’ ...

** package ‘data.table’ successfully unpacked and MD5 sums checked

** using staged installation

** libs

clang -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -I/usr/local/include -fopenmp -fPIC -Wall -g -O2 -c assign.c -o assign.o

clang: error: unsupported option '-fopenmp'

make: *** [assign.o] Error 1

ERROR: compilation failed for package ‘data.table’

  • removing ‘/Users/travis/R/Library/data.table’

  • restoring previous ‘/Users/travis/R/Library/data.table’

Error in i.p(...) :

(converted from warning) installation of package ‘data.table’ had non-zero exit status

Calls: ... with_rprofile_user -> with_envvar -> force -> force -> i.p

Execution halted

The command "Rscript -e 'deps <- remotes::dev_package_deps(dependencies = NA);remotes::install_deps(dependencies = TRUE);if (!all(deps$package %in% installed.packages())) { message("missing: ", paste(setdiff(deps$package, installed.packages()), collapse=", ")); q(status = 1, save = "no")}'" failed and exited with 1 during .

Your build has been stopped.

Appveyor:
Packages required but not available:
  'haven', 'curl', 'data.table', 'readxl', 'openxlsx', 'tibble'

Packages suggested but not available:
'bit64', 'testthat', 'knitr', 'magrittr', 'clipr', 'csvy', 'feather',
'fst', 'hexView', 'jsonlite', 'pzfx', 'readODS', 'readr', 'rmatio',
'xml2', 'yaml'


Environment: R_VERSION=release, R_ARCH=x64 and Environment: R_VERSION=oldrel, RTOOLS_VERSION=33, CRAN=http://cran.rstudio.com work fine, the problem is only on Environment: R_VERSION=devel, R_ARCH=x64, GCC_PATH=mingw_64

From this, we know that the file sniffing works on Linux and Windows. MacOS verdict awaiting fix to the problem of data.table not compiling.

…test for isfiletext()
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 9, 2019

Codecov Report

Merging #239 into master will increase coverage by <.01%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
+ Coverage   86.52%   86.52%   +<.01%     
==========================================
  Files          19       20       +1     
  Lines         957      965       +8     
==========================================
+ Hits          828      835       +7     
- Misses        129      130       +1
Impacted Files Coverage Δ
R/isfiletext.R 87.5% <87.5%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35755f0...61e6b52. Read the comment docs.

@bokov bokov mentioned this pull request Nov 14, 2019
1 of 3 tasks complete
@bokov

This comment has been minimized.

Copy link
Contributor Author

bokov commented Nov 14, 2019

Please see #236 (comment) regarding the out-of-date CI failures. Closes #236

@bokov bokov changed the title isfiletext() checks whether a file is text or binary in a cross platf… isfiletext() checks whether a file is text or binary… Nov 14, 2019
Copy link
Owner

leeper left a comment

Generally this looks useful. Some mostly stylistic but a few substantive comments.

R/isfiletext.R Outdated Show resolved Hide resolved
R/isfiletext.R Outdated Show resolved Hide resolved
R/isfiletext.R Outdated Show resolved Hide resolved
R/isfiletext.R Outdated Show resolved Hide resolved
R/isfiletext.R Outdated Show resolved Hide resolved
tests/testthat/test_isfiletext.R Outdated Show resolved Hide resolved
tests/testthat/test_isfiletext.R Show resolved Hide resolved
R/isfiletext.R Outdated Show resolved Hide resolved
@bokov bokov requested a review from leeper Dec 20, 2019
Copy link
Owner

leeper left a comment

Generally good. Nearly there!

R/isfiletext.R Outdated Show resolved Hide resolved
R/isfiletext.R Outdated Show resolved Hide resolved
R/isfiletext.R Outdated Show resolved Hide resolved
bokov added 2 commits Dec 20, 2019
@bokov

This comment has been minimized.

Copy link
Contributor Author

bokov commented Dec 21, 2019

Appveyor timing out on R install step, hopefully temporary problem

@leeper leeper merged commit ffc7c80 into leeper:master Dec 21, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@leeper

This comment has been minimized.

Copy link
Owner

leeper commented Dec 21, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.