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

style with styler #1

Closed
wants to merge 2 commits into from
Closed

Conversation

lorenzwalthert
Copy link

Styling is done with a version of styler containing both r-lib/styler#150 and r-lib/styler#149.

According to the vignette programming with dplyr, there is no space around bang-bang operators.

@lorenzwalthert
Copy link
Author

Reference: r-lib/styler#93.

}
if (length(inplace)) {
quo <- quo(withCallingHandlers(!! quo, !!! inplace))
quo <- quo(withCallingHandlers(!!quo, !!!inplace))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hadley @lionel-: Should we add the space after the bang-bangs to the tidyverse style guide?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened tidyverse/style#38 for that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I add a space but I think other people don't.

R/dots.R Outdated
@@ -97,7 +97,8 @@ dots_clean_empty <- function(dots, is_empty, ignore_empty) {

if (n_dots) {
which <- match.arg(ignore_empty, c("trailing", "none", "all"))
switch(which,
switch(
which,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep the which on the same line with the switch( here? This is one of the few special cases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could but it would involve creating a text_before column in our parse data and it is not covered by the tidyverse style guide. In addition, I was not sure which of the below versions would be correct:

switch(cond,
  x, 
  y
)

or

switch(cond,
       x, 
       y
)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the switch and the arguments are part of the same nest (=sub-tibble of the nested parse data)?

I'm fine with

switch(cond,
  x, 
  y
)
ifelse(cond,
  true_part,
  false_part
)

A simpler solution that seems enough for now: The line break after the opening paren is added only if strict = TRUE. I'm not sure what the current setup is.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the former. The idea is that it is a special case of this style:

list(
  a,
  b
)

where the main arguments go onto the first line. There could be one, two or three of them. A rough guide is that anything before ... is on the first line, but that does not need to be always the case.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lionel- for chiming in. I like the idea that anything before the triple dots may stay after the opening paren, we should be able to cover most cases (apart from ifelse()). Unfortunately, doing this accurately also seems to require knowledge about the callee's argument list, which we don't have yet.

Let's simply avoid adding a line break for strict = FALSE .

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the function call itself not.

test <- c("switch(abc,", "xy,z)")
styler:::create_tree(test)
#>                                              levelName
#> 1  ROOT (token: short_text [lag_newlines/spaces] {id})
#> 2   °--expr:  [0/0] {20}                              
#> 3       ¦--expr:  [0/0] {3}                           
#> 4       ¦   °--SYMBOL_FUNCTION_CALL: switc [0/0] {1}  
#> 5       ¦--'(': ( [0/0] {2}                           
#> 6       ¦--expr:  [0/0] {6}                           
#> 7       ¦   °--SYMBOL: abc [0/0] {4}                  
#> 8       ¦--',': , [0/0] {5}                           
#> 9       ¦--expr:  [1/0] {12}                          
#> 10      ¦   °--SYMBOL: xy [0/0] {10}                  
#> 11      ¦--',': , [0/0] {11}                          
#> 12      ¦--expr:  [0/0] {17}                          
#> 13      ¦   °--SYMBOL: z [0/0] {15}                   
#> 14      °--')': ) [0/0] {16}

But anyway, we can go into the first child and check text if pd$token_before[2] is a "SYMBOL_FUNCTION_CALL", so we don't need text_before. I think we do something similar in other cases.

@lorenzwalthert
Copy link
Author

This was for testing purposes only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants