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

Argument / option to apply re-style code with styler #108

Closed
jennybc opened this Issue Sep 24, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@jennybc
Member

jennybc commented Sep 24, 2017

I'm switching from formatR::tidy_source() to styler::style_text() (#94). The most specific motivation is styler's ability to handle tidy eval code. Currently reprex re-styles code that came via expression, because the srcref approach mutilates leading whitespace.

Now that I'm playing with styler I wonder if reprex should re-style all code. At least by default?

I could add an argument to control re-styling, that defaults to TRUE. BTW I have plans to make several arguments like this settable via an option, so they're easy to set in a startup file.

This would have the effect of putting good code style in front of the eyeballs of all reprex authors and consumers.

cc @hadley @jimhester

@hadley

This comment has been minimized.

Member

hadley commented Sep 25, 2017

I'm generally not in favour of restyling code, but here it might make sense.

@jimhester

This comment has been minimized.

Member

jimhester commented Sep 25, 2017

I actually don't think we need to style the code in any case, I am going to send a PR to retrieve the original formatting for expressions via the 'wholeSrcref' attribute in the captured expression.

@jimhester

This comment has been minimized.

Member

jimhester commented Sep 25, 2017

#112 has adjusted code that preserves the original expression formatting.

I would turn off automatic formatting for expressions and have reformatting only as an option (disabled by default).

@jennybc

This comment has been minimized.

Member

jennybc commented Sep 26, 2017

#112 is now included in #114, fyi.

Once that's merged, I either have to restyle expression code or remove the common leading whitespace, because preserving the whitespace verbatim, breaks the use of roxygen comments to insert text.


Input with natural indenting, i.e. what RStudio wants you to do:

reprex({
  #' # A Big Heading
  #'
  #' Look at my cute example. I love the
  #' [reprex](https://github.com/tidyverse/reprex#readme) package!
  y <- 1:4
  mean(y)
})

Output

#' # A Big Heading
#'
#' Look at my cute example. I love the
#' [reprex](https://github.com/tidyverse/reprex#readme) package!
y <- 1:4
mean(y)
#> [1] 2.5

Input with manual (lack of) indenting, so roxygen comments still work:

reprex({
#' # A Big Heading
#'
#' Look at my cute example. I love the
#' [reprex](https://github.com/tidyverse/reprex#readme) package!
y <- 1:4
mean(y)
})

Output

A Big Heading

Look at my cute example. I love the reprex package!

y <- 1:4
mean(y)
#> [1] 2.5
@jennybc

This comment has been minimized.

Member

jennybc commented Sep 26, 2017

Here's my new proposal: add an argument for code re-styling (and give it the option treatment soon) and have it default to FALSE in general and TRUE for expression input.

@jimhester

This comment has been minimized.

Member

jimhester commented Sep 26, 2017

I think in that case you should just remove the common leading whitespace. glue::trim() would be an easy way to do the trimming, or you can do it with a regex

x <- c(
  "  some text",
  "    some indented text",
  "  some more text")

trim_whitespace <- function(x) {
  m <- regexpr("^(\\s*)", x)
  leading_whitespace <- regmatches(x, m)
  num <- min(nchar(leading_whitespace))
  substring(x, num + 1)
}
trim_whitespace(x)
#> [1] "some text"            "  some indented text" "some more text"

y <- c("text without leading whitespace")
trim_whitespace(y)
#> [1] "text without leading whitespace"

z <- c(
  "text without leading whitespace",
  "  test with leading whitespace"
)
trim_whitespace(z)
#> [1] "text without leading whitespace" "  test with leading whitespace"

@jennybc jennybc changed the title from Apply styler to all code? to Argument / option to apply re-style code with styler Sep 26, 2017

@jennybc jennybc closed this in 5e7cd8b Oct 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment