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

Fix style_file() so it works for a vector of files from different directories #522

Merged
merged 12 commits into from Jun 19, 2019

Conversation

@lorenzwalthert
Copy link
Collaborator

commented Jun 16, 2019

Closes #521 by not changing working directory to directory where the file lays. The bug was long undetected because setwd() fails silently on a character vector within withr::with_dir() and is particularly relevant for #467, e.g. when R files in R/ and tests/testthat are staged simultaneously.

Note that this is an API change in the sense that now, the path to the directory where the files lay are printed, not just the file names, which also affects the invisible return values of the stylers that contain the path.

withr::with_dir(
  "~/datasciencecoursera/styler", {
    out <- styler::style_file(c("R/utils-files.R", "tests/testthat/strict/strict-out.R"))
    out
  }
)
#> Styling  2  files:
#>  R/utils-files.R                    ✔ 
#>  tests/testthat/strict/strict-out.R ✔ 
#> ────────────────────────────────────────
#> Status   Count   Legend 
#> ✔    2   File unchanged.
#> ℹ    0   File changed.
#> ✖    0   Styling threw an error.
#> ────────────────────────────────────────
#> # A tibble: 2 x 2
#>   file                               changed
#>   <chr>                              <lgl>  
#> 1 R/utils-files.R                    FALSE  
#> 2 tests/testthat/strict/strict-out.R FALSE

Created on 2019-06-19 by the reprex package (v0.2.1)

This change has implications for testing also. To make a comparison with an exact expectation, in the tests, we temporarily change the working directory to keep file paths and ruler width (that depends on the path) identical on all platforms. See ?styler:::catch_style_file_output. Testing for the cat() outputs of the public API was refactored also in this PR.

The new utility rds_to_version() can be used to convert rds objects to the old serialization format to ensure CI passing. They are introduced in this PR because this is a use case where we need to udpate current reference values. Closes #524.

do not change working directory when using style_file()
It has no obvious pros, but the cons are: Can't style multiple files if they are not in the same directory (and need overhead to adapt with status quo to it) + the messages of styling only show the filename, not the path, which might be confusing.

@lorenzwalthert lorenzwalthert force-pushed the lorenzwalthert:fix-style-file branch from 61401b9 to 2414ddf Jun 16, 2019

@lorenzwalthert lorenzwalthert force-pushed the lorenzwalthert:fix-style-file branch from 2414ddf to 1a9a854 Jun 16, 2019

@lorenzwalthert lorenzwalthert changed the title Fix style file Fix style_file() so it works for a vector of files from different directories Jun 16, 2019

@lorenzwalthert lorenzwalthert force-pushed the lorenzwalthert:fix-style-file branch from c3d8ba1 to c442be2 Jun 17, 2019

@lorenzwalthert lorenzwalthert force-pushed the lorenzwalthert:fix-style-file branch from c442be2 to 30dc4cd Jun 17, 2019

testing infrastructure refactoring, documenting, also remove ruler de…
…pending on whether unicode is available before comparing captured output (because ruler length depends on max(nchar(files), 0) which depends on path structure on plattform which is not stable.
@codecov-io

This comment has been minimized.

Copy link

commented Jun 19, 2019

Codecov Report

Merging #522 into master will decrease coverage by 0.12%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #522      +/-   ##
==========================================
- Coverage   90.18%   90.06%   -0.13%     
==========================================
  Files          39       40       +1     
  Lines        1732     1741       +9     
==========================================
+ Hits         1562     1568       +6     
- Misses        170      173       +3
Impacted Files Coverage Δ
R/testing.R 68.31% <0%> (-1.39%) ⬇️
R/transform-files.R 100% <100%> (ø) ⬆️
R/testing-public-api.R 100% <100%> (ø)
R/ui.R 100% <100%> (ø) ⬆️
R/utils.R 83.33% <0%> (-5.56%) ⬇️

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 67225ed...0dba22d. Read the comment docs.

@lorenzwalthert lorenzwalthert force-pushed the lorenzwalthert:fix-style-file branch from 11cb46f to d5e92cb Jun 19, 2019

execute styling within temp dir because then the path to the file is …
…just the file name and we don't need to bother about dir() returning C:\\xyz on windows and file.path() returning C:/ ect.

@lorenzwalthert lorenzwalthert force-pushed the lorenzwalthert:fix-style-file branch 2 times, most recently from c5d9e8c to edb5208 Jun 19, 2019

@lorenzwalthert lorenzwalthert force-pushed the lorenzwalthert:fix-style-file branch from edb5208 to 0dba22d Jun 19, 2019

@lorenzwalthert lorenzwalthert merged commit 42f1a41 into r-lib:master Jun 19, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@lorenzwalthert lorenzwalthert deleted the lorenzwalthert:fix-style-file branch Jun 19, 2019

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