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

Add support for .Rnw files #434

Merged
merged 7 commits into from Nov 3, 2018

Conversation

Projects
None yet
3 participants
@jonmcalder
Copy link
Contributor

commented Oct 16, 2018

To address #431

@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

commented Oct 17, 2018

Good start :-)

@codecov-io

This comment has been minimized.

Copy link

commented Oct 20, 2018

Codecov Report

Merging #434 into master will decrease coverage by 0.21%.
The diff coverage is 88.63%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #434      +/-   ##
=========================================
- Coverage   91.02%   90.8%   -0.22%     
=========================================
  Files          34      34              
  Lines        1571    1599      +28     
=========================================
+ Hits         1430    1452      +22     
- Misses        141     147       +6
Impacted Files Coverage Δ
R/addins.R 0% <0%> (ø) ⬆️
R/ui.R 100% <100%> (ø) ⬆️
R/set-assert-args.R 92.3% <100%> (ø) ⬆️
R/utils.R 68.75% <100%> (-9.03%) ⬇️
R/testing.R 73.46% <100%> (ø) ⬆️
R/transform-code.R 96% <96.29%> (+1.4%) ⬆️

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 34f82cd...6682607. Read the comment docs.

@jonmcalder

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

@lorenzwalthert I've completed a (rough) first stab at this and there should be enough here now for an initial review.

Obviously will need to add some more tests eventually, but would first like to see to what extent you're happy with the structure of the implementation. I'm guessing you might want me to refactor some of this a little.

#' an Rmd file and recombines the resulting (styled) code chunks with the text
#' chunks.
#'
#' @param lines A character vector of lines from an Rmd file

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Oct 21, 2018

Collaborator

tidyverse style requires full stops.

This comment has been minimized.

Copy link
@jonmcalder

jonmcalder Oct 23, 2018

Author Contributor

👍 (have applied this correction elsewhere too)

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Oct 23, 2018

Collaborator

Thanks.

#' @importFrom purrr flatten_chr
#' @keywords internal
transform_rmd <- function(lines, transformer_fun) {
transform_mixed(lines, transformer_fun, filetype = "Rmd")

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Oct 21, 2018

Collaborator

We could also use purrr::partial(). Not sure what's preferred here. Maybe documenting these helper functions in transform_mixed() would suffice using #' @describeIn. Otherwise, I think we can use #' @inheritParams to avoid duplication.

This comment has been minimized.

Copy link
@jonmcalder

jonmcalder Oct 23, 2018

Author Contributor

Yeah I think purrr:partial() is more elegant. I've now reworked this since I no longer feel the explicit transform_rmd() and transform_rnw() functions are necessary.

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Oct 23, 2018

Collaborator

👍

separate_chunks <- function(lines, filetype = "Rmd") {
if(filetype == "Rmd") {
r_raw_chunks <- identify_rmd_raw_chunks(lines)
} else {

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Oct 21, 2018

Collaborator

This else case implicitly implies that filetype == "Rnw", because of the logic above but you need context for this. I'd suggest to make it explicit:

Suggested change
} else {
} else if (filetype == "Rnw") {

This comment has been minimized.

Copy link
@jonmcalder

jonmcalder Oct 23, 2018

Author Contributor

👍

get_knitr_pattern <- function(lines, filetype = "Rmd") {
if(filetype == "Rnw") {
knitr::all_patterns[["rnw"]]
} else {

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Oct 21, 2018

Collaborator

Here also I'd be explicit on the else part.

This comment has been minimized.

Copy link
@jonmcalder

jonmcalder Oct 23, 2018

Author Contributor

👍

R/ui.R Outdated
@@ -188,7 +188,7 @@ prettify_any <- function(transformers,
)
}

#' Style `.R` and/or `.Rmd` files
#' Style `.R` and/or `.Rmd` and.or `.Rnw` files

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Oct 21, 2018

Collaborator

typo here I think after and.

This comment has been minimized.

Copy link
@jonmcalder

jonmcalder Oct 23, 2018

Author Contributor

👍

#' to contain R code.
#' @inheritParams separate_chunks
#' @keywords internal
identify_rnw_raw_chunks <- function(lines) {

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Oct 22, 2018

Collaborator

I am sure you realized this adds quite a bit of duplication :-) However, maybe you are right and we should accept this because any other approach that involves re-using code seems to make things more complicated. I went back with git blame and I realized that the handling Rmd chunks that could both entail non-R engines as well code without an engine were added later (#313, #386). Maybe we should first look into simplify identify_r_raw_chunks()? Do you see a simple fix how we can structure the function to work for both symmetric (Rnw) and the asymmetric (Rmd) case?

This comment has been minimized.

Copy link
@jonmcalder

jonmcalder Oct 23, 2018

Author Contributor

Yeah I didn't particularly like this duplication but couldn't think of a better way at the time - probably just a bit lazy! 😄

I think I've done it a little better this time around.

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Oct 23, 2018

Collaborator

Agree.

#' @param transformer_fun A styler transformer function
#' @param lines A character vector of lines from an Rmd or Rnw file.
#' @param transformer_fun A styler transformer function.
#' @param filetype A string indicating the filetype (Rmd or Rnw).

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Oct 23, 2018

Collaborator

I think here we could inherit lines and filetype from separate_chunks() with #' @inheritParams.

This comment has been minimized.

Copy link
@jonmcalder

jonmcalder Oct 23, 2018

Author Contributor

👍

context("public API - Rnw in style_file()")

test_that("styler can style Rnw file", {
capture_output(expect_false({

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Oct 23, 2018

Collaborator

So for testing, I suggest we stick to the infrastructure we established before for consistency, i.e. use test_collection(). I think this would require to adapt test_collection to reckon the .Rnw pattern:
https://github.com/r-lib/styler/blob/master/R/testing.R#L30

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Oct 23, 2018

Collaborator

This is used almost everywhere, e.g. here: tests/testthat/test-rmd.R

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Oct 23, 2018

Collaborator

Sorry, did not see the tests below. -.-

This comment has been minimized.

Copy link
@jonmcalder

jonmcalder Oct 23, 2018

Author Contributor

😄

@lorenzwalthert
Copy link
Collaborator

left a comment

As far as edge cases go: Can we add a test with a blank code chunk? Any other edge cases on top of your mind?

@jonmcalder

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

Actually I didn't really have any edge cases in mind and it's been years since I last used Sweave. Just figured more test coverage would be needed.

Aren't blank code chunks already covered by the styling test on random2.Rnw?

} else if (filetype == "Rnw") {
starts <- grep(pattern$chunk.begin, lines, perl = TRUE)
ends <- grep(pattern$chunk.end, lines, perl = TRUE)
is_r_code <- rep(TRUE, length(start))

This comment has been minimized.

Copy link
@lorenzwalthert

lorenzwalthert Oct 23, 2018

Collaborator

I think we are missing an s here

Suggested change
is_r_code <- rep(TRUE, length(start))
is_r_code <- rep(TRUE, length(starts))

Not causing any problems (except from R CMD CHECK) because

start
#> function (x, ...) 
#> UseMethod("start")
#> <bytecode: 0x107af2ed8>
#> <environment: namespace:stats>

This comment has been minimized.

Copy link
@jonmcalder

jonmcalder Oct 23, 2018

Author Contributor

Well spotted!

@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

commented Oct 23, 2018

Aren't blank code chunks already covered by the styling test on random2.Rnw?

Agree to the extent that a blank line is inserted if there is none. But this was the case already with .Rmd so I guess it's fine. 😜

@lorenzwalthert
Copy link
Collaborator

left a comment

Cool. Looks good to me. Last thing: (how) do we need to adapt style_pkg and friends? Are vignettes written in sweave?

@lorenzwalthert lorenzwalthert changed the title WIP: Add support for .Rnw files Add support for .Rnw files Oct 26, 2018

@jonmcalder

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2018

Apologies for the delayed response. Before R 3.0 vignettes had to be written in Sweave, but it's obviously become much less common now. My suggestion would be to handle .Rnw the same way as .Rmd in style_pkg(), and style_dir().

Show resolved Hide resolved R/ui.R

@lorenzwalthert lorenzwalthert merged commit 0154a18 into r-lib:master Nov 3, 2018

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
@lorenzwalthert

This comment has been minimized.

Copy link
Collaborator

commented Nov 3, 2018

Thanks a lot @jonmcalder. Nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.