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

Conversation

RichardJActon
Copy link

Implements parallel styling of files by the style_pkg & style_dir functions addressing issue #277 using the future and future.apply packages.

Example of how a user would get parallel styling:

library(future)
plan(multisession, workers = 4)
style_pkg()

Copy link
Collaborator

@lorenzwalthert lorenzwalthert left a 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
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?

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().

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

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

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

@RichardJActon
Copy link
Author

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?

@lorenzwalthert
Copy link
Collaborator

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 😄

@lorenzwalthert lorenzwalthert mentioned this pull request Mar 23, 2020
@lorenzwalthert
Copy link
Collaborator

Any progress on that @RichardJActon. I can fix the merge conflicts after you're done.

@lorenzwalthert
Copy link
Collaborator

Closing due to inactivity and new developments in #682.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants