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

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

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 changed the title Fix style file Fix style_file() so it works for a vector of files from different directories Jun 16, 2019
…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
Copy link

codecov-io 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.

…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 fix-style-file branch 2 times, most recently from c5d9e8c to edb5208 Compare June 19, 2019 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants