Handle styling of an unsaved active file #243

jonmcalder · 2017-10-12T21:34:32Z

Fixes #235

lorenzwalthert · 2017-10-12T21:58:58Z

R/addins.R

@@ -3,7 +3,15 @@
 #' Helper function for RStudio Addin.
 style_active_file <- function() {
  context <- get_rstudio_context()
-  style_file(context$path, style = tidyverse_style)
+  if (context$path != "") {


What's the advantage of case distinction here? Can't we just use rstudioapi to handle all cases?

I just assumed I should avoid changing the current behaviour for the case when the active file has been saved (i.e that it is styled and saved).

Was your intention with #235 to just modify (without saving) in all cases?

For consistency, I suggest to stick with rstudioapi throughout. See #235 (comment).

lorenzwalthert · 2017-10-12T21:59:12Z

Thanks for working on that Jon.

lorenzwalthert · 2017-10-22T19:22:37Z

Let's do away with these startup messages and implement the default to not save after styling. We can maybe add a help file "styler_addins" (with an alias "style_fule" and friends) where we describe each addin and the option quickly and make reference to .Rprofile.

krlmlr · 2017-10-23T08:08:19Z

On that note, I've noticed that styling a chunk in an .Rmd file pulls the closing backticks ``` directly after the selection, is this still true after this change?

The name of the menu item "Style active region" may be confusing, can we change to "Style selection"?

jonmcalder · 2017-10-23T12:28:44Z

Thanks for picking that up @krlmlr - was that using the add-in implementation from this branch or an earlier version? I didn't do extensive testing and haven't looked at this recently so will need to check again later when I'm home.

I agree with the suggested name change - @lorenzwalthert if you are happy with that too can I include it in this PR?

krlmlr · 2017-10-23T12:41:48Z

This was with styler from master.

lorenzwalthert · 2017-10-23T15:22:27Z

Yes, I am happy with the name change in this PR

lorenzwalthert · 2017-10-26T14:32:49Z

As far as the lost end-of-chuck line breaks goes, I think that's not related primarily to this PR, but styler drop lines at the end anyways,

reprex::reprex_info()
#> Created by the reprex package v0.1.1.9000 on 2017-10-26

styler::style_text("1++1\n")
#> 1 + +1

That stems from the fact that we don't have an end token. See discussions around start tokens in #218, #44 and #4. We always have a final line blank if we use functions that use enc::transform_lines_enc() because it does add that line for us, but here we use style_text(). Why do you @krlmlr think this is undesirable? If we want to ensure a blank line at the end of every code chuck, we could just add it after serialisation.

lorenzwalthert · 2017-10-26T14:33:29Z

@jonmcalder please take note of #264 and adapt the PR accordingly.

krlmlr · 2017-10-26T15:07:37Z

The closing backticks in a .Rmd need to remain on a separate line, and if the selection ends with newlines, styler should keep them.

lorenzwalthert · 2017-10-26T15:27:49Z

Also @jonmcalder can you please rebase on master because then we can test on .Rmd files too. @krlmlr I can't reproduce the behaviour you describe with the current master branch. I only see blank lines removed when I use the Addin for styling highlighted code, but the back-ticks remain on a separate line, i.e. they don't follow the code on the same line as the code ends. With highlighting code and back-ticks, I can't perform the styling as the parse data is invalid.

Fixes r-lib#235

lorenzwalthert · 2017-10-27T07:53:24Z

Ok @krlmlr now I get what you mean. If I recall correctly we had that problem with style_selection() before we used rstudioapi::modifyRange(). It think we used this workaround to adjust the range for this case. However, we could probably do that in a slightly more elegant way by adding a line break at the end of the styled expression before inserting it again.

jonmcalder · 2017-10-27T21:51:21Z

Ok documentation added/updated - I think this should be ready for review now @lorenzwalthert.

My understanding is that the above conversation around refining the usage of rstudioapi::modifyRange() within style_selection() and/or updating style_text() to handle Rmd content is out of scope and this will need to be addressed in separate PR's.

So just to summarize the content of this PR again:

update style_active_file() to allow for styling of untitled/unsaved files
rename style_active_region() to style_selection()
add option to automatically save after styling via these addins (using an environment variable)
update function documentation

lorenzwalthert · 2017-10-29T16:10:52Z

R/addins.R

+#' @section Auto-Save Option:
+#' By default, both of the RStudio Addins will apply styling to the (selected)
+#' file contents without saving changes. Automatic saving can be enabled by
+#' setting the environment variable \code{save_after_styling} to \code{TRUE}.


We use markdown roxygen, so can you please change

\code{...}

to

`...`

here and elsewhere in the PR?

Oops! Sorry, old habits...

lorenzwalthert · 2017-10-29T16:12:39Z

R/addins.R

-#' Helper function for RStudio Addin. This function is complicated because of
-#' one thing: You can highlight also just parts of lines.
+
+#' @describeIn styler_addins Styles the highlighted region
 #' @importFrom rlang seq2


I think the rlang import is redundant. Can you remove as part of this PR?

lorenzwalthert · 2017-10-29T16:47:35Z

Ok, thanks @jonmcalder. For me it's ok to solve the problem with the back-ticks brought up by @krlmlr later. I already solved it in some branch, so I will add the commits after we merged this PR.

One last thing: Would you mind updating R/styler.R to contain a reference to to the RStudio Addin help file (line 10) just as for the other stylers?

…tyler_addins docs

lorenzwalthert · 2017-10-30T07:12:14Z

R/styler.R

@@ -7,7 +7,8 @@
 #'  * [style_file()] to style a single .R file.
 #'  * [style_dir()] to style all .R files in a directory.
 #'  * [style_pkg()] to style the source files of an R package.
-#'  * An RStudio Addin to style the active file .R file and the current package.
+#'  * [styler_addins] (RStudio Addins) to style either selected code or the


Can you add two braces after styler_addins (as it is done with style_pkg() etc. above) to not just make a reference, but also use code font markup?

I was deliberately trying to avoid making the reference look like a function since it's not. Does that distinction not really matter here?

Ok, that actually makes sense. You are right.

lorenzwalthert · 2017-10-30T20:24:01Z

Thanks

- Adapt documentation (#290). - Add roundtrip (#287). - Fix AppVeyor builds. - Fix token insertion / comment interaction (#279). - Clarify labelling strategy (#285). - Fixing and extending Rstudioaddins (#283). - Fix eq assign parsing (#276). - style_files -> vectorized style_file (#273). - Refactoring (#270). - Fix CI (#275). - Fix covr (#274). - Renaming files (#271). - Handle styling of an unsaved active file (#243). - Test R 3.1 and R 3.2 (#249). - Allow empty {} without line break (#261). - Wrap expr in expr before enclosing with curly braces (#263). - Avoid checking for hard-coded dot (#262). - Account for dependency renaming (utf8 changed to enc) (#264). - Indention of function declaration and closing braces (#260). - Only remove line break before closing with strict option (#252).

lorenzwalthert reviewed Oct 12, 2017

View reviewed changes

jonmcalder added 3 commits October 26, 2017 22:08

Handle styling of an unsaved active file

066c68f

Fixes r-lib#235

Use rstudioapi with optional save

ff792ad

Remove startup message, rename style_active_region() & add save option

3f07d8d

jonmcalder force-pushed the style-active-file branch from 5a59832 to 3f07d8d Compare October 26, 2017 20:49

Update documentation for styler_addins

fd4a420

lorenzwalthert reviewed Oct 29, 2017

View reviewed changes

Use Roxygen markdown, remove redundant dependency, add reference to s…

8dfa171

…tyler_addins docs

lorenzwalthert reviewed Oct 30, 2017

View reviewed changes

lorenzwalthert merged commit 37dd58f into r-lib:master Oct 30, 2017

Handle styling of an unsaved active file #243

Handle styling of an unsaved active file #243

Uh oh!

Conversation

jonmcalder commented Oct 12, 2017

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!

lorenzwalthert commented Oct 12, 2017

Uh oh!

lorenzwalthert commented Oct 22, 2017

Uh oh!

krlmlr commented Oct 23, 2017

Uh oh!

jonmcalder commented Oct 23, 2017

Uh oh!

krlmlr commented Oct 23, 2017

Uh oh!

lorenzwalthert commented Oct 23, 2017

Uh oh!

lorenzwalthert commented Oct 26, 2017

Uh oh!

lorenzwalthert commented Oct 26, 2017

Uh oh!

krlmlr commented Oct 26, 2017

Uh oh!

lorenzwalthert commented Oct 26, 2017

Uh oh!

lorenzwalthert commented Oct 27, 2017

Uh oh!

jonmcalder commented Oct 27, 2017

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!

lorenzwalthert commented Oct 29, 2017

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!

lorenzwalthert commented Oct 30, 2017

Uh oh!

Uh oh!