-
Notifications
You must be signed in to change notification settings - Fork 73
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @RichardJActon. A few minor things. Can you also add a test or two? I don't know how many core the CI machines have, but you can detect them with parallel::detectCores()
(to avoid importing the future package explicitly, see also https://stackoverflow.com/questions/28954991/whether-to-use-the-detectcores-function-in-r-to-specify-the-number-of-cores-for). Since the APIs messages are different, you could implement one for style_file(). Best add them in styler/tests/testthat/test-public_api.R
and make sure to first capture active future strategy and restore it afterwards.
@@ -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 |
There was a problem hiding this comment.
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?
transform_files <- function(files, | ||
transformers, | ||
include_roxygen_examples, | ||
root = ".") { |
There was a problem hiding this comment.
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()
.
@@ -216,7 +226,8 @@ prettify_any <- function(transformers, | |||
recursive, | |||
exclude_files, | |||
exclude_dirs, | |||
include_roxygen_examples) { | |||
include_roxygen_examples, | |||
path = ".") { |
There was a problem hiding this comment.
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.
@@ -91,7 +95,8 @@ prettify_pkg <- function(transformers, | |||
filetype, | |||
exclude_files, | |||
exclude_dirs, | |||
include_roxygen_examples) { | |||
include_roxygen_examples, | |||
pkg_root = ".") { |
There was a problem hiding this comment.
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
@@ -28,7 +28,8 @@ Imports: | |||
tibble (>= 1.4.2), | |||
tools, | |||
withr (>= 1.0.0), | |||
xfun (>= 0.1) | |||
xfun (>= 0.1), | |||
future.apply |
There was a problem hiding this comment.
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
)
I'm working on some tests. I'm not quite sure what you mean by 'make sure to first capture active future strategy and restore it afterwards' could you clarify? |
Withint a testthat test test_that(..., {
old_plan <- future::plan()
on.exit(future::plan(old_plan)
# you test (including setting a plan)
}) Not sure it works as expected but try 😄 |
Any progress on that @RichardJActon. I can fix the merge conflicts after you're done. |
Closing due to inactivity and new developments in #682. |
Implements parallel styling of files by the
style_pkg
&style_dir
functions addressing issue #277 using thefuture
andfuture.apply
packages.Example of how a user would get parallel styling: