Don't reindent function calls #149

lorenzwalthert · 2017-08-22T18:06:43Z

Removes function update_indention_ref_fun_call() from style_tidyverse() and adds functions to (1) add line break after opening brace of function calls if the call is multi-line and (2) adds a line break before closing brace if the call is multi-line. Closes #134 .

krlmlr

Great, thanks!

krlmlr · 2017-08-22T19:24:31Z

R/rules-line_break.R

+           except_token = NULL) {
+  if (!is_function_call(pd)) return(pd)
+  npd <- nrow(pd)
+  if (npd == 3) {


Why is this shortcut important?

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.

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

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.

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.

krlmlr · 2017-08-22T19:25:14Z

tests/testthat/indention_operators/eq_assign-out.R

+  new = (
+    2
+  )
+) # FIXME: Add line break between parens (#125)


Do we still need this FIXME?

krlmlr · 2017-08-22T19:27:16Z

tests/testthat/indention_token_dependent/token_dependent_complex-out.R

-     )
-     )
+  1,
+  2, c(


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?

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?

krlmlr · 2017-08-22T21:03:32Z

R/rules-line_break.R

+  }
+  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)


exept -> except

krlmlr · 2017-08-22T21:05:47Z

R/rules-line_break.R

+           except_token = NULL) {
+  if (!is_function_call(pd)) return(pd)
+  npd <- nrow(pd)
+  if (npd == 3) {


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.

codecov · 2017-08-23T07:37:30Z

Codecov Report

Merging #149 into master will decrease coverage by 1.82%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
- Coverage   87.68%   85.86%   -1.83%     
==========================================
  Files          19       19              
  Lines         755      778      +23     
==========================================
+ Hits          662      668       +6     
- Misses         93      110      +17

Impacted Files	Coverage Δ
R/rules-spacing.R	`95.16% <ø> (ø)`	⬆️
R/style_guides.R	`97.46% <100%> (+0.2%)`	⬆️
R/rules-line_break.R	`100% <100%> (ø)`	⬆️
R/reindent.R	`63.04% <0%> (-36.96%)`	⬇️

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 db6d4b1...ae7c748. Read the comment docs.

- Vignette on customizing styler (#145). - No line break after `switch()` and friends (#152). - Remove flat relicts completely (#151). - Don't reindent function calls and break line correctly for multi-line calls (#149). - Set space between "=" and "," (#150). - Make R CMD Check perfect (#148). - Adding tests for exception handling with invalid parse data (#139). - Fix indention by checking for all potential triggers (#142). - Fix un-indention (#135). - Support wide characters (#130). - No spaces around :, :: and :::. - Redesigning the API (#123). - Solve eq_sub indention in general (#125). - Minor refactorings. - Re-indent token-dependent (#119). - Supporting more indention patterns. - Allow raw indention. - Definitively fixing eol issue with comments. - Infrastructure. - Flattening out the parse table. - New rule: no space after ! -> !!! for tidyeval. - Fix spacing around '{'. - Don't drop tokens! Fixes #101. - EOL spaces in empty comments (and in general) (#98). - mal-indention in conditional statement due to wrong specification of indent_without_paren) (#95). - Complicated indentions based on arithmetic and special operators (#96). - indention interaction on with assignment operator and other operators (#97).

lorenzwalthert force-pushed the no_reindention_calls branch 2 times, most recently from 278001c to 4c5a13c Compare August 22, 2017 18:10

lorenzwalthert changed the title ~~No reindention calls~~ Don't reindent function calls Aug 22, 2017

lorenzwalthert requested a review from krlmlr August 22, 2017 18:10

lorenzwalthert mentioned this pull request Aug 22, 2017

style with styler krlmlr/rlang#1

Closed

lorenzwalthert added 4 commits August 22, 2017 21:06

add rule to break lines correctly if expr is call and multi-line

2ed63fa

adapt tests

01256e7

Space before ',' if preceding token is '='

2d34511

add tests

48f6866

lorenzwalthert force-pushed the no_reindention_calls branch from 4c5a13c to 01256e7 Compare August 22, 2017 19:08

lorenzwalthert mentioned this pull request Aug 22, 2017

{ / } and line breaks in function calls #125

Closed

krlmlr approved these changes Aug 22, 2017

View reviewed changes

krlmlr reviewed Aug 22, 2017

View reviewed changes

lorenzwalthert added 3 commits August 23, 2017 08:50

outsource empty function call case.

44b50c4

spelling and documenting

6e8b482

remove fixme

cd805de

lorenzwalthert force-pushed the no_reindention_calls branch from 2b7df99 to cd805de Compare August 23, 2017 07:09

Merge branch 'master' into no_reindention_calls

ae7c748

lorenzwalthert merged commit 5868c21 into r-lib:master Aug 23, 2017

krlmlr mentioned this pull request Aug 24, 2017

Line breaks in function calls with named arguments tidyverse/style#39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Don't reindent function calls #149

Don't reindent function calls #149

Uh oh!

lorenzwalthert commented Aug 22, 2017

Uh oh!

krlmlr left a comment

Uh oh!

krlmlr Aug 22, 2017

Uh oh!

lorenzwalthert Aug 22, 2017

Uh oh!

krlmlr Aug 22, 2017

Uh oh!

lorenzwalthert Aug 22, 2017

Uh oh!

krlmlr Aug 22, 2017

Uh oh!

lorenzwalthert Aug 23, 2017

Uh oh!

krlmlr Aug 22, 2017

Uh oh!

lorenzwalthert Aug 22, 2017

Uh oh!

krlmlr Aug 22, 2017

Uh oh!

lorenzwalthert Aug 23, 2017

Uh oh!

krlmlr Aug 23, 2017

Uh oh!

krlmlr Aug 22, 2017

Uh oh!

krlmlr Aug 22, 2017

Uh oh!

codecov bot commented Aug 23, 2017

Uh oh!

Uh oh!

-                   )
-                   )
+,
+, c(

Don't reindent function calls #149

Don't reindent function calls #149

Uh oh!

Conversation

lorenzwalthert commented Aug 22, 2017

Uh oh!

krlmlr left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 23, 2017

Codecov Report

Uh oh!

Uh oh!