Addin to style highlighted region #143

lorenzwalthert · 2017-08-21T17:02:00Z

This closes #107. It seems a bit complicated because of the fact that you can highlight also just parts of lines. Did not really know how we could add formal testing to that. One way would be to make context an argument of style_active_region() with default find_active_context() so we can supply some file region for testing purposes.

codecov · 2017-08-21T17:09:32Z

Codecov Report

Merging #143 into master will decrease coverage by 0.58%.
The diff coverage is 82.14%.

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
- Coverage   93.85%   93.26%   -0.59%     
==========================================
  Files          19       20       +1     
  Lines         748      802      +54     
==========================================
+ Hits          702      748      +46     
- Misses         46       54       +8

Impacted Files	Coverage Δ
R/ws.R	`96% <ø> (+7.11%)`	⬆️
R/serialized_tests.R	`100% <100%> (ø)`	⬆️
R/addins.R	`81.81% <81.81%> (ø)`

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 dd23ac9...76ca7ec. Read the comment docs.

krlmlr

I wonder if the addin should operate in interactive mode, because there seems to be no feedback otherwise, e.g., when styling the entire package.

Also, styling the active region doesn't seem to work if an incomplete expression is selected. We could search for the sub-tibble that fully contains the selected region and style only this, but that's worth a separate issue.

lorenzwalthert · 2017-08-22T08:08:59Z

Yes, I thought about that too. I think it gives feedback, e.g. "No files changed" appears on the console but I think the problem is also that the error handling is not very good. Same with the other Addins, although I think the "style active file"-Addin is the most important one. The reason I have not explored interactive Addins is that I can't do that as part of Milestone CRAN due to time constraints. Should we leave it as is or should we keep all Addins for a future CRAN release?

krlmlr · 2017-08-22T19:06:38Z

"Interactive" just means that it runs in the active R session, and blocks it -- a simple change to inst/rstudio/addins.dcf, not a GUI or anything.

lorenzwalthert · 2017-08-22T19:09:27Z

Ok, I see. Let's do that then.

lorenzwalthert · 2017-08-23T07:06:45Z

It actually does not work the way I thought because we use the RStudio API to get the active document. If we set interactive = TRUE, the Addin is executed in the console and can't get the active document context. So this mode would only work as a short cut for style_pkg() but not for styling the active file or the highlighted region on the active file.

krlmlr · 2017-08-23T07:24:57Z

rstudioapi::getActiveDocumentContext() works for me when I run it from the console. What's the difference to an interactive add-in?

lorenzwalthert · 2017-08-23T07:30:12Z

Oh now it works, had to restart RStudio also, not just rebuild the package and restart R. With regard to tests, do you think we should make context an argument as stated above?

krlmlr · 2017-08-24T11:55:34Z

You could implement:

style_active_region <- function() {
  context <- find_active_context()
  all_text <- utf8::read_lines_enc(context$path)
  ...
}

get_rstudio_context <- function() {
  rstudioapi::getActiveDocumentContext()
}

and then in the test ask

test_that("...", {
  result <- mockr::with_mock(
    get_rstudio_context = function() structure(list(...), class = "document_context"),
    style_active_region()
  )
  expect...(result, ...)
})

lorenzwalthert · 2017-08-27T11:01:40Z

I tried to use mockr but when running this test_that() statement interactively, it gave me an error with mockr::with_mock(), but not with testthat::with_mock(). When running via devtools::test(f = "public"), both did not give an error. I used testhat for now, maybe @krlmlr can have a look why that happends? Also, I did not mock get_rstudio_context() but find_active_context() since this function is the one that gets called by the function we want to test and I was not sure whether it was possible to mock functions that are not directly called by the function we want to test.

krlmlr

I'm fine with postponing automated tests. Maybe split this PR so that the testing is added in a separate commit/branch?

krlmlr · 2017-08-28T16:53:41Z

R/addins.R

+#'
+#' Helper function for RStudio Addin.
+style_active_file <- function() {
+  file <- rstudioapi::getActiveDocumentContext()$path


Do you need to call find_active_context() here to allow mocking?

If we want I guess so. So far, only style_actige_region is tested but we can add a test here too to increase coverage.

krlmlr · 2017-08-28T16:54:08Z

R/addins.R

+  path <- context$path
+  start <- context$selection[[1]]$range$start
+  end <- context$selection[[1]]$range$end
+  if (all(start == end)) return()


... and then return a list with only one component here?

I think this should rather be stop("No region selected") instead of return().

Even if we want to call this for getting the active file?

No then not. But currently, we don't use find_active_context() for styling files. Should we? If yes, then the error message should not be implemented within the function find_active_context, I agree.

It's currently only used for styling regions within files.

Ok, I adapted it so it is used in both cases.

krlmlr · 2017-08-28T16:54:48Z

tests/testthat/test-public_api.R

@@ -17,3 +17,37 @@ test_that("styler can style file", {
 })

 # style_active_file() must be tested manually.
+test_that("...", {


krlmlr · 2017-08-28T16:58:10Z

tests/testthat/test-public_api.R

+  before_styling <- utf8::read_lines_enc(paste0(base, "/xyzaddin/addin_region-in.R"))
+  reference <- utf8::read_lines_enc(paste0(base, "/xyzaddin/addin_region-out.R"))
+  result <- expect_error(
+    testthat::with_mock(


mockr::with_mock() should really work here, because testthat::with_mock() might not at some point. Can you please simplify the code (e.g., extract the hard-coded "document_context" structure to a variable) and try again? For troubleshooting the error message is helpful.

mockr::with_mock() works using devtools::test() but not interactively while testthat::with_mock() works in both sistuations, which is why I have not used mockr yet. Can you have a look at why that is the case?

What do you mean with extracting hard-coded "document_context" structure to a variable? Like this?

doc_cont <- structure(...) testthat("...", { result <- mockr::with_mock( get_rstudio_context = function() doc_cont, ...

Unfortunately, there is no way to create the structure with a function call directly.

Yes, something like this code.

You could try mockr::with_mock(..., envir = asNamespace("styler")) .

Ok, so assign the variable elsewhere in the name space? Or should leave the declaration where it is?

krlmlr · 2017-08-28T16:59:52Z

R/addins.R

+#' [rstudioapi::getActiveDocumentContext()].
+#'
+find_active_context <- function() {
+  context <- rstudioapi::getActiveDocumentContext()


I thought we could just create a simple forwarder to rstudioapi::getActiveDocumentContext() in our package, and mock this forwarder:

get_rstudio_context <- function() { rstudioapi::getActiveDocumentContext() }

mockr still works if functions are called indirectly from our package (it patches all functions in our package).

Ok yes I see. That's what I a say unsure about (see comment above).

krlmlr · 2017-08-29T11:06:38Z

Do you need more input here?

lorenzwalthert · 2017-08-30T06:11:56Z

Can you review the updated PR?

krlmlr

Looks good, thanks. If you call devtools::release(), one of the first things it does is a spell check of the documentation, this will find the typos we've missed.

krlmlr · 2017-08-30T07:02:24Z

R/styler.R

-#'  according to a style guide. See the INDEX for more information.
-#'
+#'  according to a style guide.
+#'  The fo,lowing functions can be used for styling:


krlmlr · 2017-08-30T07:03:50Z

tests/testthat/test-public_api.R


+  before_styling <- utf8::read_lines_enc(paste0(base, "/xyzaddin/addin_region-in.R"))


Maybe just copy to a temporary directory?

dir <- tempfile("styler") dir.create(dir) file.copy(".../addin_region-out.R", dir)

The current approach seems slightly brittle if the tests aborts before the original contents have been written.

Ok, right. So if I understand you right, we copy the *-in.R* to the temporary directory, style it there and compare it with the -*out. in the permanent directory?

krlmlr · 2017-08-30T07:05:19Z

R/addins.R

+#'
+#' Essentially adding the parts before the highlighted area on the line where
+#' the higlighted area starts to the styled expression. Also, after the styled
+#' expression, the remainer of that line that was not styled is added.


krlmlr

Looks good, but we might need yet another iteration.

krlmlr · 2017-08-30T13:36:11Z

README.md

@@ -23,7 +23,7 @@ There are a few variants of `style_text()`:
 -   `style_file()` styles a single .R file.
 -   `style_dir()` styles all .R files in a directory.
 -   `style_pkg()` styles the source files of an R package.
-   An RStudio Addin that styles the active file .R file
+-   RStudio Addins for styling the active file, styling current package and styling the highlighted code region.


the current package?

Well, the package which has its root in the current working directory (if any). Could not find a better formulation for it. Any ideas?

styling current package -> styling the current package

krlmlr · 2017-08-30T13:36:37Z

tests/testthat/test-public_api.R

+
+# style_active_file() must be tested manually.
+test_that("styling active region works", {
+  reference <- utf8::read_lines_enc( paste0(base, "/xyzaddin/addin_region-out.R"))


file.path() better explains your intentions. Even better, a small function

testthat_file <- function(...) { file.path(rprojroot::find_testthat_root_file("public-api"), ...) }

instead of the base variable. Here and elsewhere.

krlmlr · 2017-08-30T13:40:57Z

tests/testthat/test-public_api.R

@@ -21,3 +21,39 @@ test_that("styler does not return error when there is no file to style", {
 })


+##  ............................................................................
+
+dir <- tempfile("styler")


All these variables seem to fit better in the test_that() block below.

krlmlr · 2017-08-30T13:41:49Z

tests/testthat/test-public_api.R

@@ -21,3 +21,39 @@ test_that("styler does not return error when there is no file to style", {
 })


Would the preceding tests change the original files?

No, they should not, because the input is already styled. The tests are only to check whether the call returns an error or not. It's a rather superficial test.

These files will then need an update whenever the style guide changes, but that's fine with me.

krlmlr · 2017-08-30T13:43:07Z

R/addins.R

+  list(
+    start = fract_of_line_start,
+    end = fract_of_line_end,
+    body = lines_not_to_style


body seems a misnomer if it's

the lines that are not overlapping with the highlighted text.

So maybe just lines_not_to_style?

krlmlr · 2017-08-30T16:14:28Z

README.md

@@ -23,7 +23,7 @@ There are a few variants of `style_text()`:
 -   `style_file()` styles a single .R file.
 -   `style_dir()` styles all .R files in a directory.
 -   `style_pkg()` styles the source files of an R package.
-   An RStudio Addin that styles the active file .R file
+-   RStudio Addins for styling the active file, styling current package and styling the highlighted code region.


krlmlr · 2017-08-30T16:16:39Z

R/serialized_tests.R

+#' Create the path to a test that file
+#' @param ... Arguments passed to [file.path()] to construct the path after
+#'   ".../tests/testthat/"
+testthat_file <- function(...) {


This might be better suited in a tests/testthat/helper-*.R file.

ups, forgot to knit .Rmd to md.

Ok, but then we won't get roxygen2 documentation, right?

Yes, I forgot. Let's keep it here.

lorenzwalthert · 2017-08-30T19:18:23Z

@krlmlr Do you want to have a final look? I think now we got them all 😄

@jimhester

- Hotfix: utf8 should not be verbose (#245). - Allow styling of Rmd files(#233). - Remove duplicate @family (#244). - Fixing token insertion (#242). - Capitalize Addin titles (#241). - Explicit `NULL` creation to make styler compatible with R3.2.0 (#237). - Improve vignettes (#232). - Allow exclusion of files with `style_pkg()` and `style_dir()`. - Correct styling with long strings (#230). - Add tools for re-indenting tokens (#217). - Math token spacing (#221). - Remove outdated line and col information (#218). - Empty input for styling does not cause an error (#227, #228). - Tools to insert tokens + application on `if`-`else` clauses (#212). - Improve example in documentation (#222). - Fix spacing around in (#214). - Maintenance: renaming functions / files, extend helper, documentation, if_else etc. (#204). - Disallow line break after ( for function calls (#210). - Preserve space between `!` and bang (#209). - Simplify RStudio Addin (#208, #211). - Indention based on square brackets (#207). - Add vignette on introducing styler (#203). - Indent function declaration without curly braces correctly (#202). - Fix indention in if-else statement (#200). - Sorting key (#196). - Use safe sequences (#197). - Fix space between two commas (#195). - Keep single-line pipes on one line (#74). - Remove tidyr and dplyr dependency (#182, #183, @jimhester). - Fix parsing inconsistency (#188). - Substitute create filler (#190). - Introducing class vertical (#189). - Adapt line break rules (#172). - Fix `R CMD check` (#185). - Force argument evaluation for proper error handling (#179). - Add nonstrict version of set_space_before_comment (#174). - Add installation instructions to README (#175). - Addin to style highlighted region (#143). - Improve spelling (#168). - Add coverage badge - Change badge from WIP to active - Add the number of files to message (#166). - Improve documentation (#164). - Add informative messages while styling files (#165). - More examples in help file (#161). - No line breaks after pipe if comment is next token (#160). - Fixing spacing around `!` (non-bang-bang) (#157). - Finalize function documentation (#154). - Review vignette (#158). - Update bang-bang rule (#156).

lorenzwalthert requested a review from krlmlr August 21, 2017 17:02

lorenzwalthert force-pushed the rstudioaddin branch 4 times, most recently from fa679c7 to d85a54d Compare August 21, 2017 17:08

lorenzwalthert force-pushed the rstudioaddin branch from d85a54d to fadc3c3 Compare August 21, 2017 17:09

krlmlr reviewed Aug 22, 2017

View reviewed changes

lorenzwalthert force-pushed the rstudioaddin branch from edbae6b to b65296d Compare August 27, 2017 10:55

lorenzwalthert requested a review from krlmlr August 28, 2017 08:21

krlmlr reviewed Aug 28, 2017

View reviewed changes

lorenzwalthert force-pushed the rstudioaddin branch 2 times, most recently from bde246e to fc426a9 Compare August 29, 2017 11:45

lorenzwalthert requested a review from krlmlr August 29, 2017 11:48

krlmlr approved these changes Aug 30, 2017

View reviewed changes

lorenzwalthert mentioned this pull request Aug 30, 2017

devtools spelling #168

Merged

lorenzwalthert force-pushed the rstudioaddin branch 2 times, most recently from 688e4a5 to 5516951 Compare August 30, 2017 11:01

lorenzwalthert added 2 commits August 30, 2017 15:33

Addin to style highlighted code regions

bb5c54e

make Addins interactive.

387fc7c

lorenzwalthert added 5 commits August 30, 2017 15:33

adding simple test for highlighted region via with_mock.

332ef6b

update description

3331d1a

update readme

c44b04c

improve addin and tests for it

506f13e

add mockr to DESCRIPTION

04957c3

lorenzwalthert force-pushed the rstudioaddin branch from 5516951 to 7b556c7 Compare August 30, 2017 13:34

lorenzwalthert changed the title ~~Add in to style highlighted region~~ Addin to style highlighted region Aug 30, 2017

typos and use temp dir for test

58681c9

lorenzwalthert force-pushed the rstudioaddin branch from 7b556c7 to 58681c9 Compare August 30, 2017 13:43

krlmlr approved these changes Aug 30, 2017

View reviewed changes

lorenzwalthert added 2 commits August 30, 2017 16:12

better path creation for tests

c4a746d

spelling and naming

9f645ff

lorenzwalthert requested a review from krlmlr August 30, 2017 15:58

krlmlr approved these changes Aug 30, 2017

View reviewed changes

lorenzwalthert added 3 commits August 30, 2017 18:22

knit to md.

78d20b9

remove empty test

130bcd6

roxygenize

76ca7ec

krlmlr approved these changes Aug 30, 2017

View reviewed changes

lorenzwalthert merged commit ef10c29 into r-lib:master Aug 31, 2017

lorenzwalthert mentioned this pull request Oct 11, 2017

style active unsaved file #235

Closed


		before_styling <- utf8::read_lines_enc(paste0(base, "/xyzaddin/addin_region-in.R"))

		@@ -21,3 +21,39 @@ test_that("styler does not return error when there is no file to style", {
		})

Addin to style highlighted region #143

Addin to style highlighted region #143

Uh oh!

Conversation

lorenzwalthert commented Aug 21, 2017

Uh oh!

codecov bot commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

krlmlr left a comment

Choose a reason for hiding this comment

Uh oh!

lorenzwalthert commented Aug 22, 2017

Uh oh!

krlmlr commented Aug 22, 2017

Uh oh!

lorenzwalthert commented Aug 22, 2017

Uh oh!

lorenzwalthert commented Aug 23, 2017

Uh oh!

krlmlr commented Aug 23, 2017

Uh oh!

lorenzwalthert commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krlmlr commented Aug 24, 2017 • edited by lorenzwalthert Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lorenzwalthert commented Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krlmlr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krlmlr commented Aug 29, 2017

Uh oh!

lorenzwalthert commented Aug 30, 2017

Uh oh!

krlmlr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 21, 2017 •

edited

Loading

lorenzwalthert commented Aug 23, 2017 •

edited

Loading

krlmlr commented Aug 24, 2017 •

edited by lorenzwalthert

Loading

lorenzwalthert commented Aug 27, 2017 •

edited

Loading