Skip to content

/rio

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
+36 −2

Conversation

@bokov
Copy link
Contributor

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:

  • if suggesting code changes or improvements, open an issue first (#223 )
  • 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

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
@bokov
Added dynamic argument checking for 'readODS:read_ods()' as proposed in
Loading status checks…
3e186f4 
#223. Also more detailed tests.
@codecov-io

This comment has been minimized.

Sign in to view
Copy link

commented Sep 23, 2019 •
edited

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.
@bokov bokov referenced this pull request Sep 23, 2019
Open
@jennybc

This comment has been minimized.

Sign in to view
Copy link

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 🤷‍♀️
This was referenced Sep 25, 2019
Open
Open
@leeper
leeper requested changes Oct 2, 2019
View changes
Copy link
Owner

left a comment

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
Fixed coding style issues from #225#pullrequestreview-296157799 in im…
Loading status checks…
a39354c 
…port_methods.R
@bokov bokov requested a review from leeper Oct 2, 2019
@bokov

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

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 leeper:master Oct 3, 2019
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leeper leeper

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@bokov @codecov-io @jennybc @leeper
You can’t perform that action at this time.