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

Add support for pzfx files #211

Merged
merged 5 commits into from Oct 2, 2019
+39 −0

Conversation

@billdenney
Copy link
Contributor

commented Jun 27, 2019

Fixes #205
billdenney added 2 commits Jun 27, 2019
@billdenney
Add support for pzfx files
Loading status checks…
1f5c3a2
@billdenney
Completion of .pzfx addition by documenting in NEWS.md
Loading status checks…
e559302
@billdenney billdenney referenced this pull request Jun 27, 2019
Merged
@billdenney
Give message if pzfx test is skipped due to missing library
Loading status checks…
549b0d4
@billdenney

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jun 27, 2019

This gets an error that Namespace dependency not required: "pzfx". I put it into "Suggests", but I apparently need to do something differently. Can you suggest the required fix?
@Yue-Jiang
Yue-Jiang suggested changes Sep 21, 2019
View changes
Copy link

left a comment

Thanks for making this happen! I think the error has to do with the @importFrom, I think for suggested packages they don't use that line, for example see how yaml is handled.
R/export_methods.R Outdated
@@ -299,3 +299,10 @@ export_delim <- function(file, x, fwrite = TRUE, sep = "\t", row.names = FALSE,
requireNamespace("clipr")
clipr::write_clip(content = x, row.names = row.names, col.names = col.names, sep = sep, ...)
}

#' @importFrom pzfx write_pzfx

This comment has been minimized.

Sign in to view
Copy link
@Yue-Jiang

Yue-Jiang Sep 21, 2019

This line will result in a requirement for 'pzfx' in the Imports: field, which causes the error Namespace dependency not required: "pzfx". I think removing this line should fix it.
R/import_methods.R Outdated
@@ -421,3 +421,10 @@ function(file,
requireNamespace("clipr")
clipr::read_clip_tbl(x = clipr::read_clip(), header = header, sep = sep, ...)
}

#' @importFrom pzfx read_pzfx

This comment has been minimized.

Sign in to view
Copy link
@Yue-Jiang

Yue-Jiang Sep 21, 2019

and this line too
billdenney added 2 commits Sep 25, 2019
@billdenney
Merge branch 'master' into add-pzfx
Loading status checks…
9dd9e9a 
# Conflicts:
#	DESCRIPTION
@billdenney
Remove pzfx as import requirement
Loading status checks…
8ad4e53
@billdenney

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Sep 25, 2019

Thanks for those pointers. I made the suggested changes, and now there are only notes with devtools::check().
@codecov-io

This comment has been minimized.

Sign in to view
Copy link

commented Sep 25, 2019 •
edited

Codecov Report

Merging #211 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #211      +/-   ##
==========================================
+ Coverage   82.43%   82.51%   +0.08%     
==========================================
  Files          18       18              
  Lines         871      875       +4     
==========================================
+ Hits          718      722       +4     
  Misses        153      153
Impacted Files Coverage Δ
R/import.R 91.66% <ø> (ø) ⬆️
R/export_methods.R 89.43% <100%> (+0.15%) ⬆️
R/import_methods.R 80.2% <100%> (+0.2%) ⬆️

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 be234aa...8ad4e53. Read the comment docs.
@leeper leeper merged commit 6dce742 into leeper:master Oct 2, 2019
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 82.43%)
Details
codecov/project 82.51% (+0.08%) compared to be234aa
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@leeper

This comment has been minimized.

Sign in to view
Copy link
Owner

commented Oct 2, 2019

Thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yue-Jiang Yue-Jiang

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