checkr
checkr copied to clipboard
peterhurford
Added @importFrom utils head and namespace to call of head(); that is…
Added @importFrom utils head and namespace to call of head(); that is… utils::head()
I am getting a clean Travis-ci build with my branch. I have tried to get all of the edits into this group of pull requests.
All of the additions of @importFrom ... force the NAMESPACE file to change. I figure yours is being automatically generated so I did not do a PR for that.
@rmsharp Thanks again for the contribution! Let me try to clean up Travis. 👍
@rmsharp Can you run devtools::document() locally and commit the results? I rebased this PR to go off of a branch where I think I fixed Travis.
Peter,
Your skill level is far beyond mine so forgive me if I am missing something.
You have exported the function validate, however, the following line occurs in test-validate.R expect_true(validate(checkr:::validate %is% "function"))
This test passes as does the same test without the namespace::: prefix. expect_true(validate(validate %is% "function”))
Which should be used?
R. Mark Sharp 7526 Meadow Green St. San Antonio, TX 78251 mobile: 210-218-2868 [email protected]
On Oct 10, 2017, at 9:12 AM, Peter Hurford [email protected] wrote:
@rmsharp Thanks again for the contribution! Let me try to clean up Travis. 👍
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
Will do. I actually have already converted the documents, but held off thinking you might want to do that yourself and not mess with my PR.
Mark R. Mark Sharp 7526 Meadow Green St. San Antonio, TX 78251 mobile: 210-218-2868 [email protected]
On Oct 10, 2017, at 10:16 AM, Peter Hurford [email protected] wrote:
@rmsharp Can you run devtools::document() locally and commit the results? I rebased this PR to go off of a branch where I think I fixed Travis.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
Agreed. Sorry, I have three branches going although I had incorporated that in the one I have on Travis-ci, I failed to get it into the one I can use for PR.
Mark R. Mark Sharp 7526 Meadow Green St. San Antonio, TX 78251 mobile: 210-218-2868 [email protected]
On Oct 10, 2017, at 10:53 AM, Peter Hurford [email protected] wrote:
@peterhurford commented on this pull request.
In NAMESPACE:
export(quickcheck) export(should_be_checked) export(validate) export(validate_) import(memoise) import(methods) +importFrom(devtools,as.package)
If we add this (which is fine), we need to move devtools from the Suggests to the Imports in the DESCRIPTION file.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
You have exported the function validate, however, the following line occurs in test-validate.R expect_true(validate(checkr:::validate %is% "function"))
This test passes as does the same test without the namespace::: prefix. expect_true(validate(validate %is% "function”))
...Well that is bizarre and not easily explained. I think removing the checkr::: is fine.
That is fine. I have a style requirement of <= 80 and I had turned it off because you have many lines over 80.
One of the joys of style :-)
R. Mark Sharp 7526 Meadow Green St. San Antonio, TX 78251 mobile: 210-218-2868 [email protected]
On Oct 10, 2017, at 10:54 AM, Peter Hurford [email protected] wrote:
@peterhurford commented on this pull request.
In R/generate.R:
@@ -72,30 +72,35 @@ random_matrix <- function(objects) { random_data_class <- sample(matrix_classes, 1) sample_data <- function(data) { sample(data, random_width * random_height, replace = TRUE) } random_data <- switch(random_data_class,
- integer = sample_data(c(objects$negative_integers, objects$positive_integers)),
- double = sample_data(c(objects$negative_doubles, objects$positive_doubles)),
- logical = sample_data(objects$logicals),
- character = sample_data(c(objects$characters, objects$utf8)),
- simple_string = sample_data(random_simple_strings(random_width * random_height,
objects = objects)))
integer = sample_data(c(objects$negative_integers, objects$positive_integers)),double = sample_data(c(objects$negative_doubles, objects$positive_doubles)),logical = sample_data(objects$logicals),character = sample_data(c(objects$characters, objects$utf8)),simple_string = sample_data(random_simple_strings(random_width * random_height,objects = objects)))I have a linter set to require all lines are under 100 characters... this style change breaks that linter.
You can run the linter locally with lintr::lint_package().
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
Again another style issue.
I think the blanks do help some, but I do not know how to change RStudio’s distaste for them.
Mark R. Mark Sharp 7526 Meadow Green St. San Antonio, TX 78251 mobile: 210-218-2868 [email protected]
On Oct 10, 2017, at 10:56 AM, Peter Hurford [email protected] wrote:
@peterhurford commented on this pull request.
In R/validate.R:
@@ -1,3 +1,7 @@ +verify_condition <- function(condition, env) {
- if (identical(eval(condition, envir = env), FALSE)) {deparse(condition)}
- else {NULL}
I personally like some space around my brackets (e.g., { NULL } not {NULL}).
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
If you want, when it gets to Travis failing solely for style issues, I can merge your branch in and clean up the style issues for you. No need for you to do all the dirty work. ;)
Can you see my Travis-ci site: https://travis-ci.org/rmsharp/checkr Builds 3, 4, and 6 were successful. 5 had the trailing whitespace.
Mark R. Mark Sharp 7526 Meadow Green St. San Antonio, TX 78251 mobile: 210-218-2868 [email protected]
On Oct 10, 2017, at 11:06 AM, Peter Hurford [email protected] wrote:
If you want, when it gets to Travis failing solely for style issues, I can merge your branch in and clean up the style issues for you. No need for you to do all the dirty work. ;)
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
@rmsharp Yep, I can see your Travis. I think you need to push those commits upstream to this PR for my Travis to pick it up.
It has several of the formatting changes that will be an irritant, but I will send them.
Mark R. Mark Sharp 7526 Meadow Green St. San Antonio, TX 78251 mobile: 210-218-2868 [email protected]
On Oct 10, 2017, at 11:52 AM, Peter Hurford [email protected] wrote:
@rmsharp Yep, I can see your Travis. I think you need to push those commits upstream to this PR for my Travis to pick it up.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
When you see those PRs you will clearly see my level of ignorance. I was fumbling around trying to get checkr to work, because I want to use it and would love to have it on CRAN.
I will be glad to do anything I can to help make the PRs more manageable, but I have not worked much with others in git. I thought I had to have a separate fork (instead of a master). That is where first 10 or so PRs came from (ourtac/checkr/rmsharp-branch).
Mark R. Mark Sharp 7526 Meadow Green St. San Antonio, TX 78251 mobile: 210-218-2868 [email protected]
On Oct 10, 2017, at 11:52 AM, Peter Hurford [email protected] wrote:
@rmsharp Yep, I can see your Travis. I think you need to push those commits upstream to this PR for my Travis to pick it up.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
When you see those PRs you will clearly see my level of ignorance. I was fumbling around trying to get checkr to work
Hey, that's totally fine. We all have to get our start somewhere!
~
because I want to use it and would love to have it on CRAN.
That's really cool to hear! I agree. I made a milestone to track progress toward CRAN publication: https://github.com/peterhurford/checkr/milestone/1
~
I will be glad to do anything I can to help make the PRs more manageable, but I have not worked much with others in git.
No worries, we can smooth everything over after by squashing commits before merging. The GitHub UI has a feature that allows this to be done easily without the command line.