Skip to content

Don't reindent function calls #149

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

Merged
merged 8 commits into from
Aug 23, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions R/rules-line_break.R
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,51 @@ remove_line_break_before_round_closing <- function(pd) {
pd
}

#' @importFrom rlang seq2
add_line_break_after_pipe <- function(pd) {
is_special <- pd$token %in% c("SPECIAL-PIPE")
pd$lag_newlines[lag(is_special)] <- 1L
pd
}


#' Set line break for multi-line function calls
#' @param pd A parse table.
#' @param except A character vector with tokens before "'('" that do not
#' @name set_line_break_if_call_is_multi_line
#' @importFrom rlang seq2
NULL

#' @describeIn set_line_break_if_call_is_multi_line Sets line break after
#' opening parenthesis.
set_line_break_after_opening_if_call_is_multi_line <-
function(pd,
except_token = NULL) {
if (!is_function_call(pd)) return(pd)
npd <- nrow(pd)
if (npd == 3) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this shortcut important?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because seq2(3, npd - 1) would throw an error otherwise. npd = 3 is the case of empty function calls likecall(). Just making sure there this does not happen:

call(
)

We could circumvent that using seq2(3, npd) instead, can't actually remember why I did not do that.

Copy link
Member

Choose a reason for hiding this comment

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

rlang::seq2(3, 2)
#> integer(0)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Uups, no error then, sorry. Buth by dropping the if condition, we cannot solve the problem because if npd = 3, and

call(
)

we want to remove the line break before the closing parent, which we can't using seq2(a, b) with a>b. Also, my suggestion to use seq(3, npd) does not make sense since

call(2
)

would then be classified as multi-line, which I think does not make sense. We should not take the closing parenthesis into account. What do you think? Not sure whether there is another way to do that then.

Copy link
Member

Choose a reason for hiding this comment

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

It looks clearer to me to remove the shortcut and add a separate rule to remove new lines for empty calls. This new rule seems to be very low priority.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, sure.

pd$lag_newlines[3] <- 0L
return(pd)
}
is_multi_line <- any(pd$lag_newlines[seq2(3, npd - 1)] > 0)
if (!is_multi_line) return(pd)
exeption_pos <- which(pd$token %in% except_token)
Copy link
Member

Choose a reason for hiding this comment

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

exept -> except

pd$lag_newlines[setdiff(3, exeption_pos)] <- 1L
pd
}


#' @describeIn set_line_break_if_call_is_multi_line Sets line break before
#' closing parenthesis.
set_line_break_before_closing_if_call_is_multi_line <- function(pd) {
if (!is_function_call(pd)) return(pd)
npd <- nrow(pd)
if (npd == 3) {
pd$lag_newlines[3] <- 0L
return(pd)
}
is_multi_line <- any(pd$lag_newlines[seq2(3, npd - 1)] > 0)
if (!is_multi_line) return(pd)
pd$lag_newlines[npd] <- 1L
pd
}
7 changes: 6 additions & 1 deletion R/style_guides.R
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ tidyverse_style <- function(scope = "tokens",
add_line_break_afer_curly_opening,
if (strict) set_line_break_before_curly_closing else
add_line_break_before_curly_closing,
partial(
set_line_break_after_opening_if_call_is_multi_line,
except_token = "COMMENT"
),
set_line_break_before_closing_if_call_is_multi_line,
add_line_break_after_pipe
)

Expand All @@ -86,7 +91,7 @@ tidyverse_style <- function(scope = "tokens",
indention_modifier <-
c(
update_indention_ref_fun_dec,
update_indention_ref_fun_call
NULL
)

create_style_guide(
Expand Down
29 changes: 29 additions & 0 deletions man/set_line_break_if_call_is_multi_line.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions man/set_space_between_eq_sub_and_comma.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
b<-function(x ){
x <- c(1,
2+ 3,
sin(pi) ) # FIXME add tidyverse-comliant rule to break after '('
sin(pi) )

if(x > 10){
return("done")
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
b <- function(x) {
x <- c(1,
2 + 3,
sin(pi)) # FIXME add tidyverse-comliant rule to break after '('
x <- c(
1,
2 + 3,
sin(pi)
)

if (x > 10) {
return("done")
Expand Down
3 changes: 2 additions & 1 deletion tests/testthat/indention_multiple/curly_and_round-out.R
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ test_that("this is a text", {
{
call(
12, 1 + 1,
26)
26
)
}
}))

Expand Down
6 changes: 4 additions & 2 deletions tests/testthat/indention_multiple/edge_mixed-out.R
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
(({
{
{
c(99,
c(
99,
1 + 1, {
"within that suff"
})
}
)
}
}
}))
Expand Down
6 changes: 4 additions & 2 deletions tests/testthat/indention_multiple/overall-out.R
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,14 @@ a <- function(x) {
26 ^ 2, # FIXME ^ operator has to be surrounded by one space (or none?!), never multiple
8,
7
)))
))
)

call(
1, 2,
23 + Inf - 99, call(
16
))
)
)
}
# comments everywhere
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
c(call(2), 1, # choosed first function name (c) such that indention is 2.
c(call(2), 1, # comment
29
)

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
c(call(2), 1, # choosed first function name (c) such that indention is 2.
c(
call(2), 1, # comment
29
)
12 changes: 7 additions & 5 deletions tests/testthat/indention_operators/eq_assign-out.R
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
switch(engine,
pdftex = {
switch(
engine,
pdftex = {
if (any) {
x
}
},
new = (
2
)) # FIXME: Add line break between parens (#125)
new = (
2
)
) # FIXME: Add line break between parens (#125)
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this FIXME?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope.


{
a <-
Expand Down
6 changes: 4 additions & 2 deletions tests/testthat/indention_operators/not_first_trigger-out.R
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
) %>%
j()

a <- c(x, y,
z) %>%
a <- c(
x, y,
z
) %>%
k()

a + (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ call(call( # comment
3, 4
))

call(call(1, # comment
3
call(call(
1, # comment
3
))
Original file line number Diff line number Diff line change
Expand Up @@ -2,30 +2,39 @@ call(call(
2
))

call(call(1,
2))
call(call(
1,
2
))
# multi-line: no indention based on first vall
call(a(b(c({
}))))

call(
call(
2
),
5
)


call(call(
2),
5)


call(call(1,
2, c(
3
)))

call(1,
call2(3, 4, call(3,
4, call(5, 6, call(
2
)
)
)
)
1,
2, c(
Copy link
Member

Choose a reason for hiding this comment

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

We seem to need a rule that requires a newline before multi-line function arguments. Do we have a self-contained writeup for the new function call rules? Maybe open a PR to the tidystyle repo so that it's not lost?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, we don't have such a rule yet. So if I understand you correctly, you think

call(
  2, 
  3, c(
    2, 
    3
))

Should be styled as

call(
  3, 
  3, 
  c(
    very_long_such, 
    that_we_cant_put_it_on_one_line
  )
)

If we can't put the whole sub-expression c(...) on one line?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

3
)
))

call(
1,
call2(3, 4, call(
3,
4, call(5, 6, call(
2
)
)
)
)
)

# comment lala
Expand Down
Loading