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

Added dynamic argument checking for 'readODS:read_ods()' #225

Merged
merged 2 commits into from
Oct 3, 2019

Conversation

bokov
Copy link
Contributor

@bokov bokov commented Sep 23, 2019

Added dynamic argument checking for 'readODS:read_ods()' as proposed in #223. Also more detailed tests.

Please ensure the following before submitting a PR:

Note: was I supposed to increment NEWS.md to rio 0.5.21 or add my changes to the 0.5.20 entry?

  • 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

Only pre-existing error, not caused by this PR

@codecov-io
Copy link

codecov-io commented Sep 23, 2019

Codecov Report

Merging #225 into master will increase coverage by 0.35%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
+ Coverage   82.43%   82.78%   +0.35%     
==========================================
  Files          18       18              
  Lines         871      889      +18     
==========================================
+ Hits          718      736      +18     
  Misses        153      153
Impacted Files Coverage Δ
R/import_methods.R 81.73% <100%> (+1.73%) ⬆️

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 70d26ca...a39354c. Read the comment docs.

@jennybc
Copy link

jennybc commented Sep 23, 2019

I'm watching a series of related issues and PRs here re: ....

The tidyverse/r-lib packages are using the ellipsis package for similar functionality, in case that holds any interest. Yes, it would be a dependency. But it would also eliminate the need to grow your own solution here. YMMV 🤷‍♀️

Copy link
Contributor

@leeper leeper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - just a couple of nits.

R/import_methods.R Outdated Show resolved Hide resolved
R/import_methods.R Outdated Show resolved Hide resolved
@bokov bokov requested a review from leeper October 2, 2019 14:41
@bokov
Copy link
Contributor Author

bokov commented Oct 2, 2019

One of the build environments failed during setup, prior to deployment of package:

Travis...

Installing R

49.44s$ sudo add-apt-repository -y "ppa:marutter/rrutter3.5"

gpg: keyring `/tmp/tmp2oopqjqz/secring.gpg' created

gpg: keyring `/tmp/tmp2oopqjqz/pubring.gpg' created

gpg: requesting key B04C661B from hkp server keyserver.ubuntu.com

Error: retrieving gpg key timed out.

gpg: /tmp/tmp2oopqjqz/trustdb.gpg: trustdb created

gpg: key B04C661B: public key "Launchpad PPA for marutter" imported

gpg: Total number processed: 1

gpg:               imported: 1  (RSA: 1)

OK

The command "sudo add-apt-repository -y "ppa:marutter/rrutter3.5"" failed and exited with 1 during .


Appveyor maybe this part ...

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'

@leeper leeper merged commit f3b3c2d into gesistsa:master Oct 3, 2019
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.

None yet

4 participants