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

Parallel styling of files by style_pkg & style_dir (#277) #617

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion DESCRIPTION
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ Imports:
tibble (>= 1.4.2),
tools,
withr (>= 1.0.0),
xfun (>= 0.1)
xfun (>= 0.1),
future.apply
Copy link
Collaborator

@lorenzwalthert lorenzwalthert Mar 10, 2020

Choose a reason for hiding this comment

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

For the example with librar(future), we'd also need to declare future as an import explicitly (although it will be available anyways since we import future.apply)

Suggests:
data.tree (>= 0.1.6),
digest,
Expand Down
5 changes: 5 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ Release upon request by the CRAN team.
- skip timing tests on CRAN as requested by CRAN team because they did not pass
on all machines (#603).

## New features
Copy link
Collaborator

@lorenzwalthert lorenzwalthert Mar 10, 2020

Choose a reason for hiding this comment

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

This should actually go under a new # styler 1.3.2.9000 header. Also, please reference the PR, not the issue. I also believe that style_file() will also benefit (since it is vectorized over file). Can you adapt this as well?


* `style_pkg()` and `style_dir()` gain the ability to run in parallel using
`future` backends (#277).

# styler 1.3.1

Emergency release. In case multiple expressions are on one line and only
Expand Down
13 changes: 10 additions & 3 deletions R/transform-files.R
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,22 @@
#' Invisibly returns a data frame that indicates for each file considered for
#' styling whether or not it was actually changed.
#' @keywords internal
transform_files <- function(files, transformers, include_roxygen_examples) {
transform_files <- function(files,
transformers,
include_roxygen_examples,
root = ".") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make sure the new argument root is also documented? You can @inheritParams here from transform_file and document it there. You can also add a sentence like "required because working directory set with withr::with_dir() not respected by future.apply::future_sapply().

transformer <- make_transformer(transformers, include_roxygen_examples)
max_char <- min(max(nchar(files), 0), getOption("width"))
len_files <- length(files)
if (len_files > 0L) {
cat("Styling ", len_files, " files:\n")
}

changed <- map_lgl(files, transform_file,
fun = transformer, max_char_path = max_char
changed <- future.apply::future_sapply(
files, transform_file,
fun = transformer, max_char_path = max_char, root = root
)

communicate_summary(changed, max_char)
communicate_warning(changed, transformers)
new_tibble(list(file = files, changed = changed), nrow = len_files)
Expand All @@ -43,6 +48,7 @@ transform_file <- function(path,
message_before = "",
message_after = " [DONE]",
message_after_if_changed = " *",
root = ".",
...) {
char_after_path <- nchar(message_before) + nchar(path) + 1
max_char_after_message_path <- nchar(message_before) + max_char_path + 1
Expand All @@ -53,6 +59,7 @@ transform_file <- function(path,
rep_char(" ", max(0L, n_spaces_before_message_after)),
append = FALSE
)
setwd(root)
changed <- transform_code(path, fun = fun, ...)

bullet <- ifelse(is.na(changed), "warning", ifelse(changed, "info", "tick"))
Expand Down
23 changes: 17 additions & 6 deletions R/ui-styling.R
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ NULL
#' scope = "line_breaks",
#' math_token_spacing = specify_math_token_spacing(zero = "'+'")
#' )
#' # For parallel operation:
#' library(future)
#' plan(multisession, workers = 4)
#' style_pkg()
#' }
#' @export
style_pkg <- function(pkg = ".",
Expand All @@ -82,7 +86,7 @@ style_pkg <- function(pkg = ".",
pkg_root <- rprojroot::find_package_root_file(path = pkg)
changed <- withr::with_dir(pkg_root, prettify_pkg(
transformers,
filetype, exclude_files, exclude_dirs, include_roxygen_examples
filetype, exclude_files, exclude_dirs, include_roxygen_examples, pkg_root
))
invisible(changed)
}
Expand All @@ -91,7 +95,8 @@ prettify_pkg <- function(transformers,
filetype,
exclude_files,
exclude_dirs,
include_roxygen_examples) {
include_roxygen_examples,
pkg_root = ".") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also use root as argument name and then @inheritParams from transform_files

filetype <- set_and_assert_arg_filetype(filetype)
r_files <- rprofile_files <- vignette_files <- readme <- NULL
exclude_files <- set_arg_paths(exclude_files)
Expand Down Expand Up @@ -137,7 +142,7 @@ prettify_pkg <- function(transformers,
c(r_files, rprofile_files, vignette_files, readme),
exclude_files
)
transform_files(files, transformers, include_roxygen_examples)
transform_files(files, transformers, include_roxygen_examples, pkg_root)
}

#' Style a string
Expand Down Expand Up @@ -184,6 +189,10 @@ style_text <- function(text,
#' @examples
#' \dontrun{
#' style_dir(file_type = "r")
#' # For parallel operation:
#' library(future)
#' plan(multisession, workers = 4)
#' style_dir()
#' }
#' @export
style_dir <- function(path = ".",
Expand All @@ -198,7 +207,8 @@ style_dir <- function(path = ".",
changed <- withr::with_dir(
path, prettify_any(
transformers,
filetype, recursive, exclude_files, exclude_dirs, include_roxygen_examples
filetype, recursive, exclude_files, exclude_dirs,
include_roxygen_examples, path
)
)
invisible(changed)
Expand All @@ -216,7 +226,8 @@ prettify_any <- function(transformers,
recursive,
exclude_files,
exclude_dirs,
include_roxygen_examples) {
include_roxygen_examples,
path = ".") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

also @inheritParams from `transform_files() to get path documented.

exclude_files <- set_arg_paths(exclude_files)
exclude_dirs <- set_arg_paths(exclude_dirs)
files_root <- dir(
Expand All @@ -236,7 +247,7 @@ prettify_any <- function(transformers,
}
transform_files(
setdiff(c(files_root, files_other), exclude_files),
transformers, include_roxygen_examples
transformers, include_roxygen_examples, path
)
}

Expand Down
3 changes: 2 additions & 1 deletion man/prettify_any.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions man/style_dir.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions man/style_pkg.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions man/transform_file.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion man/transform_files.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.