Add yihui_style() #449

Robinlovelace · 2018-12-04T19:46:16Z

A little context: ropensci/software-review-meta#71 (comment)

Commit: c033b7d

yihui

Haha, I just hope this function is not named after me or any particular person 😁

Perhaps a more explicit name like eq_assign_style()?

lorenzwalthert · 2018-12-04T23:27:17Z

It's called the Yihuiverse style guide. It's official now. haha. No, but it was already called kind of Yihui's style guide in #340 (comment) 😄. Of course, we can also go for a different name if @yihui does not agree and I think it could be more informative too.
Thanks for the PR @Robinlovelace. I am not sure what the current status of the issue is. We had
#340 (comment) where we agreed to add these two style guides to styler but without exporting them. Alternatively, we could export and add a life-cycle badge "experimental" to this style guide. Another question is if it should inherit all other rules from the tidyverse style guide or if the <- -> = rule is the only rule in the style guide. As you can note from the referenced issues, in particular mlr-org/mlr#2278 (comment), I am rather conservative about the topic of adding other style guides to styler. I think I need to discuss again with @krlmlr sometime soon. Also, we'd need to add unit tests to this PR in case we'd plan to merge this in styler.

Robinlovelace · 2018-12-05T07:37:24Z

Hehe I like the yihuiverse_style() function name! Seriously though, I agree it's worth thinking about an appropriate name. Originally I called the equals_style() but changed it to a more fun name for this PR. I think @yihui should definitely get a say over this. So far I can see/think of 5 options for the style name:

equals_style()
eq_assign_style()
equals_assign_style()
yihui_style()
yihuiverse_style()

I can think of various +s and -s of each name. I'd be happy with any of them and there may be more viable options I've not seen/thought of. I think you guys (primarily @lorenzwalthert and @yihui ) should have a greater say than me over the name so up to you: I'll happily change the name in the PR. Also happy to add tests and make any further changes / put the code elsewhere pending feedback.

Regarding whether

it should inherit all other rules from the tidyverse style guide

I'm not sure. Are there any other changes I should make for this to be 'yihuiverse-compliant' ✔️ @yihui hehe?

In summary: thanks for the feedback; I'll make changes pending more feedback, particular relating to the style's name.

yihui

I'm fine with kidding, but please don't make "Yihui style" or "Yihuiverse" official. I consider my style a personal taste, and I don't intend to promote it. Other than the style name, I don't have further comments.

lorenzwalthert · 2018-12-05T15:12:25Z

I'm fine with kidding, but please don't make "Yihui style" or "Yihuiverse" official.

Sure. I think we better find another name for the style guide that does not involve your name.

codecov-io · 2018-12-05T20:36:50Z

Codecov Report

Merging #449 into master will decrease coverage by 0.46%.
The diff coverage is 37.93%.

@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
- Coverage   91.02%   90.55%   -0.47%     
==========================================
  Files          36       36              
  Lines        1626     1631       +5     
==========================================
- Hits         1480     1477       -3     
- Misses        146      154       +8

Impacted Files	Coverage Δ
R/environments.R	`100% <ø> (ø)`	⬆️
R/ui.R	`100% <ø> (ø)`	⬆️
R/style-guides.R	`91.5% <0%> (-5.05%)`	⬇️
R/zzz.R	`0% <0%> (ø)`	⬆️
R/parse.R	`83.67% <0%> (ø)`	⬆️
R/rules-other.R	`100% <100%> (ø)`	⬆️
R/io.R	`100% <100%> (ø)`	⬆️
R/roxygen-examples-add-remove.R	`92.3% <100%> (ø)`	⬆️
R/transform-code.R	`96% <100%> (ø)`	⬆️
R/nested-to-tree.R	`100% <100%> (+3.57%)`	⬆️
... and 2 more

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 c7e9e91...8256cf5. Read the comment docs.

Robinlovelace · 2018-12-05T20:39:23Z

Thanks for the feedback @yihui I've updated the style name in-line with my original thinking - used equals_style() as it's more concise and I think equally understandable as equals_assign_style() and more so than eq_assign_style().

Robinlovelace · 2018-12-05T20:41:43Z

Another issue with this PR I've discovered is that the style does not work as expected:

txt = "x = c(
  1
)"
styler::style_text(txt)
#> x <- c(
#>   1
#> )
styler::style_text(txt, style = styler::equals_style)
#> x = c(
#> 1
#> )

^{Created on 2018-12-05 by the reprex package (v0.2.1)}

@lorenzwalthert any suggestions on how to fix this? At present I'm setting the tokens as follows - was thinking of naming the list but not sure if that will help. Here's the function at present in any case:

equals_style <- function() {
 create_style_guide(token = list(force_assignment_op_equals))
}

Robinlovelace · 2018-12-05T21:03:50Z

Regarding tests, I've had a first bash but not sure if this fits with your previous tests. Advice on this also appreciated @lorenzwalthert:

library(styler)
library(testthat)
txt_left_assign = "x <- 1"
txt_equals_assign = "x = 1"
res_equals_style = style_text(txt_left_assign, style = equals_style)
txt_equals_style = as.character(res_equals_style)
test_that("equals_style replaces <- with = in one-liner",{
  expect_identical(
    txt_equals_style,
    txt_equals_assign)
})

^{Created on 2018-12-05 by the reprex package (v0.2.1)}

lorenzwalthert · 2018-12-05T22:22:53Z

As far as tests go, We have a testing framework powered by test_collection(). Essentially, there is an *-in.R file and a *-out.R file. The *-in.R file is the input that is transformed and - if it matches the *-out.R file, the test has passed. You can create an *-in.R file, run devtools::test(f = "[your file]")
and an *-out.R file is generated. If the file matches your expectation, you can commit it. Note that files are overwritten and version control should be used to track failed tests. The files are placed in tests/testthat under the category they fit. Please have a look at the documentation for test_collection() and see other unit tests.
Also, I hope that you can understand that one or two tests are not sufficient to test the new style guide. There are always unexpected cases such as the one you are presenting above. styler currently has almost 150 tests for the tidyverse style guide and it fooled my over and over again. We need more comprehensive testing. Because you can't think of all cases possible, I think it is helpful to use the source code of another package, style it with the style guide you are suggesting, look at the git diff. Then, pick a few other cases and add them as to the *-in.R test suite. In your case it is pretty easy because everything should be left unchanged for a package that does not contain arrow assignment. We can't add styling of whole packages as unit tests, but we should at least do it once and cover a few cases. That's at least how I did it for oneliner, maybe you have better suggestions. In that case, let me know. At some point, we may want to build infrastructure to do that automatically with travis, but I guess its not a priority now.

As far as the indention problem goes we need use_raw_indention = TRUE:

library(styler)
force_assignment_op_equals <- function(pd) {
  to_replace <- pd$token == "LEFT_ASSIGN"
  pd$token[to_replace] <- "EQ_ASSIGN"
  pd$text[to_replace] <- "="
  pd
}

equals_style <- function() {
  create_style_guide(
    token = list(force_assignment_op_equals),
    use_raw_indention = TRUE
  )
}

txt <- "x = c(
  1
)"
style_text(txt, style = equals_style)
#> x = c(
#>   1
#> )

^{Created on 2018-12-05 by the reprex package (v0.2.1)}

lorenzwalthert · 2018-12-05T23:11:10Z

I also updated CONTRIBUTING.md to cover testing. It may contain other interesting information for you about contributing to styler.

Robinlovelace · 2018-12-06T00:00:53Z

Good news on the testing front: equals_style() has already proved useful in tidying-up style in a package with equals assignment after commits adding <- assignment: itsleeds/stats19@327e436

Some of those examples could be good in the tests I imagine.

- check on data.tree version, not R version (r-lib#450).

lorenzwalthert · 2018-12-06T15:02:45Z

See my comments in the repo there. Maybe there is an unexpected interaction with #381. Also, you could rebase on master to make the (unrelated) CI failure on R 3.2 go away now.

lorenzwalthert · 2018-12-06T15:06:01Z

See my comments in the repo there. Maybe there is an unexpected interaction with #381. Also, you could rebase on master to make the (unrelated) CI failure on R 3.2 go away now.

…quals-style

Robinlovelace · 2018-12-06T15:44:41Z

Think I've borked the rebase. Can create a new clean PR if that would make sense - suggested way forward unless there are better suggestions.

krlmlr and others added 2 commits August 17, 2017 09:38

Deploy from local build [ci skip]

1b072fa

Commit: c033b7d

Add yihui_style()

db094d6

Robinlovelace mentioned this pull request Dec 4, 2018

Other style guides #340

Closed

yihui reviewed Dec 4, 2018

View reviewed changes

yihui reviewed Dec 5, 2018

View reviewed changes

Rename equals assignment style

fa2fe69

Fix example in equals_style()

c8f47cc

lorenzwalthert added 6 commits December 5, 2018 23:30

more minimal version dependencies with namespaces

9cfe808

styling with styler

f92e84d

tidy description

3f8942d

notes on testing

a66ed8c

check on data.tree version, not R version

019c719

better namespace handling

aff4584

Add use_raw_indentation to equals_style()

a783fb8

Merge pull request r-lib#450 from lorenzwalthert/fix-data-tree

18ca662

- check on data.tree version, not R version (r-lib#450).

Robinlovelace added 3 commits December 6, 2018 15:16

Add yihui_style()

1788ac4

Rename equals assignment style

a7925de

Fix example in equals_style()

d1010d0

Robinlovelace added 4 commits December 6, 2018 15:16

Add use_raw_indentation to equals_style()

d561e2a

Merge branch 'equals-style' of github.com:robinlovelace/styler into e…

f3a3c8d

…quals-style

Fix merge conflict

f9f0e3d

Merge branch 'master' into equals-style

8256cf5

Robinlovelace closed this Dec 6, 2018

Robinlovelace mentioned this pull request Dec 6, 2018

Add equals_style() #451

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add yihui_style() #449

Add yihui_style() #449

Uh oh!

Robinlovelace commented Dec 4, 2018

Uh oh!

yihui left a comment

Uh oh!

lorenzwalthert commented Dec 4, 2018

Uh oh!

Robinlovelace commented Dec 5, 2018

Uh oh!

yihui left a comment

Uh oh!

lorenzwalthert commented Dec 5, 2018 •

edited

Loading

Uh oh!

codecov-io commented Dec 5, 2018 •

edited

Loading

Uh oh!

Robinlovelace commented Dec 5, 2018

Uh oh!

Robinlovelace commented Dec 5, 2018

Uh oh!

Robinlovelace commented Dec 5, 2018

Uh oh!

lorenzwalthert commented Dec 5, 2018 •

edited

Loading

Uh oh!

lorenzwalthert commented Dec 5, 2018

Uh oh!

Robinlovelace commented Dec 6, 2018

Uh oh!

lorenzwalthert commented Dec 6, 2018

Uh oh!

lorenzwalthert commented Dec 6, 2018

Uh oh!

Robinlovelace commented Dec 6, 2018

Uh oh!

Uh oh!

Add yihui_style() #449

Add yihui_style() #449

Uh oh!

Conversation

Robinlovelace commented Dec 4, 2018

Uh oh!

yihui left a comment

Choose a reason for hiding this comment

Uh oh!

lorenzwalthert commented Dec 4, 2018

Uh oh!

Robinlovelace commented Dec 5, 2018

Uh oh!

yihui left a comment

Choose a reason for hiding this comment

Uh oh!

lorenzwalthert commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Robinlovelace commented Dec 5, 2018

Uh oh!

Robinlovelace commented Dec 5, 2018

Uh oh!

Robinlovelace commented Dec 5, 2018

Uh oh!

lorenzwalthert commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lorenzwalthert commented Dec 5, 2018

Uh oh!

Robinlovelace commented Dec 6, 2018

Uh oh!

lorenzwalthert commented Dec 6, 2018

Uh oh!

lorenzwalthert commented Dec 6, 2018

Uh oh!

Robinlovelace commented Dec 6, 2018

Uh oh!

Uh oh!

lorenzwalthert commented Dec 5, 2018 •

edited

Loading

codecov-io commented Dec 5, 2018 •

edited

Loading

lorenzwalthert commented Dec 5, 2018 •

edited

Loading