Re-indent token-dependent #119

lorenzwalthert · 2017-08-08T21:03:22Z

This is the fourth part of #106 and it is revised significantly. This PR closes #99 and closes #66 by allowing token-dependent re-indention. This is achieved by using the visitor to add references to tokens if necessary. Tokens that don't need re-indention have NAs in the corresponding reference column. The reference is propagated to the terminals. Once the parse table is flattened out and the position of every token is defined in terms of line1, line2, col1, col2, the indention of reference token is applied to the token that should inherit the indention from the reference token and the corresponding attributes in the parse table are updated.

A transformer function that breaks lines for long function calls as suggested in the tidyverse style guide is not yet pushed to this branch. It seems rather invasive compared to the other rules. Nevertheless, I think we should include it. I was just not sure about the concrete implementation and opened #118.

codecov · 2017-08-08T21:10:03Z

Codecov Report

Merging #119 into master will increase coverage by 0.91%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage    88.6%   89.52%   +0.91%     
==========================================
  Files          18       19       +1     
  Lines         676      735      +59     
==========================================
+ Hits          599      658      +59     
  Misses         77       77

Impacted Files	Coverage Δ
R/rules-spacing.R	`94.59% <100%> (ø)`	⬆️
R/transform.R	`89.09% <100%> (+0.41%)`	⬆️
R/reindent.R	`100% <100%> (ø)`
R/get_transformers.R	`97.05% <100%> (+0.28%)`	⬆️
R/visit.R	`100% <100%> (ø)`	⬆️
R/parsed.R	`97.77% <100%> (+0.05%)`	⬆️

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 08200b8...d6a71ed. Read the comment docs.

This will create merge conflict with r-lib#119. Merge with strategy theirs.

krlmlr

Thanks, looks great.

krlmlr · 2017-08-11T09:10:56Z

tests/testthat/indention_curly_brackets/multi_line_curly_round_only-out.R

@@ -1,7 +1,7 @@
 a <- function(x) {
  x <- c(1,
    2 + 3,
-    sin(pi))
+    sin(pi)) # FIXME add tidyverse-comliant rule to break after '('


Can we always move the closing paren to a separate line?

I think we could. The question is whether that is non-invasive. I guess you only refer th the case where the function call spans over multiple lines, right? I suggest to move that discussion to #118.

krlmlr · 2017-08-11T09:15:35Z

tests/testthat/indention_token_dependent/token_dependent_comments-out.R

+))
+
+call(call(1, # comment
+          3


I'd rather have the 3 indented by two space by default, and only optionally support indention after the opening paren. I remember a discussion about this but can't currently find it.

Ok. Then, I guess the question is whether we should break after ( by default. Also, what means "by default"? I would suggest to make it dependent on the strict argument. Alternatively, we can define another level of invasiveness.
See #118.

We could indent with two space if there's a break, or optionally indent below the opening paren. Adding line breaks looks like an independent problem to me.

krlmlr · 2017-08-11T09:26:08Z

R/reindent.R

+  target_tokens <- which(flattened_pd$id %in% flattened_pd$indention_ref_id)
+
+  # needs sequential update
+  for (target_token in target_tokens) {


Reduce()? I'm not in love with this code; a vectorized solution that updates the parse table only once would be great, but we can refactor later if necessary.

I think it needs to be sequential, because some re-indention is based on token that were itself re-indented (at least in the case of function call), so I guess Reduce() (or purrr::reduce() is the way to go.

krlmlr · 2017-08-11T09:29:47Z

R/reindent.R

+#'
+#' Applies the indention level of `target_token` to all tokens that have
+#' `target_token` as a reference. This includes adding spaces to the first
+#' tokens on a line and updating the column `col1` and `col2` for all token


token -> tokens?

krlmlr · 2017-08-11T09:30:33Z

R/reindent.R

+  flattened_pd$lag_spaces[token_to_update] <-
+    flattened_pd$lag_spaces[token_to_update] + shift
+
+  # update col1 / col2


I wonder if we really need to drag along col1 and col2 (#112), but we can scrap that later.

I think we have to since indention of a token can be based on a re-indented token, hence the col information needs to be kept up to date.

krlmlr · 2017-08-11T09:31:27Z

R/reindent.R

+#' }
+#' }
+update_indention_ref_fun_dec <- function(pd_nested) {
+  if (pd_nested$token[1] == c("FUNCTION") &&


The c() looks redundant.

krlmlr · 2017-08-11T09:33:57Z

R/reindent.R

+update_indention_ref_fun_dec <- function(pd_nested) {
+  if (pd_nested$token[1] == c("FUNCTION") &&
+      nrow(pd_nested) > 3) {
+    seq <- 3:(nrow(pd_nested) - 1)


Maybe add a safe seq_from_to() helper that will return an increasing sequence starting at from and ending at to - 1, empty if from >= to? I really think : should be avoided in package code; you do check for safety but this adds complexity to the code (here and elsewhere).

Ok, that sounds like a good idea. Why should it go to to -1 and not to to?

Both work, it's a matter of taste.

A bit late, but rlang already has seq2().

Ok, I opened issue #128. Let's solve that later.

tests

- 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 pr106_5_token_dep_ind branch from f19282a to e5a321e Compare August 8, 2017 21:03

lorenzwalthert requested a review from krlmlr August 8, 2017 21:04

lorenzwalthert added a commit to lorenzwalthert/styler that referenced this pull request Aug 9, 2017

updates trees.

69f7629

This will create merge conflict with r-lib#119. Merge with strategy theirs.

lorenzwalthert added a commit to lorenzwalthert/styler that referenced this pull request Aug 9, 2017

updates trees.

22a6619

This will create merge conflict with r-lib#119. Merge with strategy theirs.

krlmlr approved these changes Aug 11, 2017

View reviewed changes

lorenzwalthert mentioned this pull request Aug 13, 2017

Create key for sorting #112

Closed

lorenzwalthert added 5 commits August 13, 2017 22:04

introduce token-dependent indention

0f1c5e3

no newline checking needed

240c415

adding test / resolving FIXMEs

64dc18d

tests

spelling and simplifying.

52a39e7

reduce instead of for loop

d6a71ed

lorenzwalthert force-pushed the pr106_5_token_dep_ind branch from 0d728d5 to d6a71ed Compare August 13, 2017 20:04

lorenzwalthert mentioned this pull request Aug 17, 2017

Safer sequences #128

Closed

lorenzwalthert merged commit 6413df8 into r-lib:master Aug 17, 2017

lorenzwalthert deleted the pr106_5_token_dep_ind branch November 21, 2017 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-indent token-dependent #119

Re-indent token-dependent #119

lorenzwalthert commented Aug 8, 2017 •

edited by krlmlr

Loading

codecov bot commented Aug 8, 2017 •

edited

Loading

krlmlr left a comment

krlmlr Aug 11, 2017

lorenzwalthert Aug 13, 2017

krlmlr Aug 11, 2017

lorenzwalthert Aug 13, 2017

krlmlr Aug 13, 2017

krlmlr Aug 11, 2017

lorenzwalthert Aug 13, 2017

krlmlr Aug 11, 2017

lorenzwalthert Aug 13, 2017

krlmlr Aug 11, 2017

lorenzwalthert Aug 13, 2017

krlmlr Aug 11, 2017

krlmlr Aug 11, 2017

lorenzwalthert Aug 13, 2017

krlmlr Aug 13, 2017

krlmlr Aug 16, 2017

lorenzwalthert Aug 17, 2017 •

edited

Loading

Re-indent token-dependent #119

Re-indent token-dependent #119

Conversation

lorenzwalthert commented Aug 8, 2017 • edited by krlmlr Loading

codecov bot commented Aug 8, 2017 • edited Loading

Codecov Report

krlmlr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzwalthert Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

lorenzwalthert commented Aug 8, 2017 •

edited by krlmlr

Loading

codecov bot commented Aug 8, 2017 •

edited

Loading

lorenzwalthert Aug 17, 2017 •

edited

Loading