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

chunks_new() wrapper #39

Merged
merged 17 commits into from
May 24, 2022
Merged

chunks_new() wrapper #39

merged 17 commits into from
May 24, 2022

Conversation

mhallal1
Copy link
Contributor

@mhallal1 mhallal1 commented May 23, 2022

closes #35 and #37

  1. A simple wrapper that returns a R6 chunks object and does not assign it to the session.
  2. Replace chunks$new with chunks_new() in all vignettes and examples.
  • in tests, use chunks$new when testing the R6 methods and chunks_new() when testing the wrappers.
  1. Change order of calls in chunks_push and chunks_push_chunks so that chunks parameter is first.

@mhallal1 mhallal1 added the core label May 23, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 23, 2022

Code Coverage Summary

Filename                Stmts    Miss  Cover    Missing
--------------------  -------  ------  -------  ----------------------------------------------------------------------------------------------------------------------------------------
R/chunk.R                 150       3  98.00%   363-369
R/chunks_pipe.R            28      25  10.71%   16-48
R/chunks.R                406      40  90.15%   334, 424, 430-433, 439, 466-467, 484-491, 500, 506-515, 561, 587, 593-594, 596, 612, 621, 630, 672, 675, 698-703, 1047, 1068, 1122, 1173
R/get_eval_details.R      113     113  0.00%    15-178
R/include_css_js.R          7       7  0.00%    12-19
R/utils.R                  24      19  20.83%   13, 17-38
TOTAL                     728     207  71.57%

Results for commit: 9821671

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented May 23, 2022

Unit Tests Summary

    1 files      3 suites   2s ⏱️
  46 tests   46 ✔️ 0 💤 0
303 runs  303 ✔️ 0 💤 0

Results for commit 8021c09.

♻️ This comment has been updated with latest results.

R/chunks.R Outdated Show resolved Hide resolved
R/chunks.R Outdated Show resolved Hide resolved
@nikolas-burkoff nikolas-burkoff self-assigned this May 23, 2022
Copy link
Contributor

@nikolas-burkoff nikolas-burkoff left a comment

Choose a reason for hiding this comment

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

A few comments but it's looking good

R/chunks.R Outdated
@@ -734,10 +734,10 @@ clone_env <- function(envir_from, envir_to) {
#'
#' @examples
#' all_chunks <- chunks$new()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we replace chunks$new() throughout codebase (except inside code/docs of the chunks R6 object itself?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

R/chunks.R Show resolved Hide resolved

testthat::test_that("chunks_new accepts NULL or empty as input", {
testthat::expect_error(chunks_new(), NA)
testthat::expect_error(chunks_new(envir = NULL), "")
Copy link
Contributor

Choose a reason for hiding this comment

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

If envir is NULL is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nop removed that.

Copy link
Contributor

@nikolas-burkoff nikolas-burkoff left a comment

Choose a reason for hiding this comment

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

I think the bottom three of these probably need changing?
image

Otherwise looks good - I wonder if there are any internal chunks tests which should use chunks$new() - i.e. if we're testing chunks$f() should we actually use chunks$new() to keep everything internal to chunks (but if testing chunks_f() then we should def use chunks_new()

@mhallal1
Copy link
Contributor Author

I think the bottom three of these probably need changing? image

Otherwise looks good - I wonder if there are any internal chunks tests which should use chunks$new() - i.e. if we're testing chunks$f() should we actually use chunks$new() to keep everything internal to chunks (but if testing chunks_f() then we should def use chunks_new()

The first and third are only changed.

Copy link
Contributor

@nikolas-burkoff nikolas-burkoff 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!

@mhallal1 mhallal1 merged commit 301fb64 into main May 24, 2022
@mhallal1 mhallal1 deleted the 35_new_chunks_wrapper@main branch May 24, 2022 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrapper for chunks$new
2 participants