Fix eq assign parsing #276

lorenzwalthert · 2017-11-08T19:33:29Z

Closes #259. As outlined in #259, we

Walk all nests
Within one nest, we split the nest into sub-nests that contain one assignment expression (e.g. a = b = c) at most.
Within every such sub-nest, we relocate the assignment expression.

However, we only do that if there is an EQ_ASSIGN token in the flat parse data since the whole procedure is rather costly and we can expect the probability that there is an EQ_ASSIGN in the parse data to be small.

codecov-io · 2017-11-08T19:47:09Z

Codecov Report

Merging #276 into master will increase coverage by 0.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #276      +/-   ##
==========================================
+ Coverage   92.95%   93.17%   +0.22%     
==========================================
  Files          28       28              
  Lines        1136     1173      +37     
==========================================
+ Hits         1056     1093      +37     
  Misses         80       80

Impacted Files	Coverage Δ
R/nest.R	`100% <100%> (ø)`	⬆️
R/relevel.R	`100% <100%> (ø)`	⬆️

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 9077126...c51460b. Read the comment docs.

krlmlr

Thanks. What a job to work around a nasty minor upstream problem...

krlmlr · 2017-11-08T20:29:17Z

R/relevel.R

+#'
+#' Although syntactically identical, [utils::getParseData()] does not produce
+#' the same hierarchy of the parse table (parent and id relationship) for `<-`
+#' and `=` (See 'Examles').


krlmlr · 2017-11-08T20:36:31Z

R/relevel.R

+    cumsum((is_eq_assign - lag(is_eq_assign, default = is_eq_assign[1])) > 2)
+  empty_seq <- rep(0, nrow(pd))
+  empty_seq[lead(pd$token == "EQ_ASSIGN", default = FALSE)] <- eq_belongs_to_block
+  block_id <- cumsum(empty_seq)


Is this the value this function returns?

Yup. Lets add

block_id

So it is explicit.

krlmlr · 2017-11-08T20:36:47Z

R/relevel.R

+  eq_expr$parent <- NA
+  non_eq_expr <- pd[-eq_ind,]
+  pd <- bind_rows(eq_expr, non_eq_expr) %>%
+    arrange(pos_id)


krlmlr · 2017-11-08T20:42:14Z

R/relevel.R

+find_block_id <- function(pd) {
+  is_eq_assign <- which(pd$token == "EQ_ASSIGN")
+  eq_belongs_to_block <-
+    cumsum((is_eq_assign - lag(is_eq_assign, default = is_eq_assign[1])) > 2)


x - lag(x) isn't much different from diff(x), and cumsum(diff(x)) is about the same as x. Does this need to be that complicated?

Well I checked diff(x) from a previous comment from you but the thing is we also need 0 to be the first element. Because with diff(), you get n - 1 elements back, not n. So we could do

c(0, diff(x)) # instad of x - lag(x, default = x[1])

Then, cumsum(c(0, diff(x)) > 2) can be simplified by pushing the zero out of cumsum, i.e.

c(0, cumsum(diff(is_eq_assign)) > 2)

Which is, I admit, simpler, but it's maybe less clear to the programmer how we arrived at that... Anyways I can change it accordingly.

Or can you simplify it even further? Cause it's not immediately clear to me how one could do that.

krlmlr

Maybe simplify a bit further?

krlmlr · 2017-11-08T21:18:09Z

R/relevel.R

+#' @param pd A parse table.
+find_block_id <- function(pd) {
+  is_eq_assign <- which(pd$token == "EQ_ASSIGN")
+  eq_belongs_to_block <- c(0, cumsum(diff(is_eq_assign)) > 2)


Are you sure it's not c(0, cumsum(diff(is_eq_assign) > 2)) ?

krlmlr · 2017-11-08T21:19:00Z

R/relevel.R

+#' `EQ_ASSING` already belongs to the `EQ_ASSING` after it.
+#' @param pd A parse table.
+find_block_id <- function(pd) {
+  is_eq_assign <- which(pd$token == "EQ_ASSIGN")


Naming: is_ hints to a logical, maybe idx_?

krlmlr · 2017-11-08T21:20:57Z

R/relevel.R

+  eq_belongs_to_block <- c(0, cumsum(diff(is_eq_assign)) > 2)
+
+  empty_seq <- rep(0, nrow(pd))
+  empty_seq[lead(pd$token == "EQ_ASSIGN", default = FALSE)] <- eq_belongs_to_block


empty_seq[idx_eq_assign + 1] <- ... ?

it's a minus 😄 .

- Adapt documentation (#290). - Add roundtrip (#287). - Fix AppVeyor builds. - Fix token insertion / comment interaction (#279). - Clarify labelling strategy (#285). - Fixing and extending Rstudioaddins (#283). - Fix eq assign parsing (#276). - style_files -> vectorized style_file (#273). - Refactoring (#270). - Fix CI (#275). - Fix covr (#274). - Renaming files (#271). - Handle styling of an unsaved active file (#243). - Test R 3.1 and R 3.2 (#249). - Allow empty {} without line break (#261). - Wrap expr in expr before enclosing with curly braces (#263). - Avoid checking for hard-coded dot (#262). - Account for dependency renaming (utf8 changed to enc) (#264). - Indention of function declaration and closing braces (#260). - Only remove line break before closing with strict option (#252).

lorenzwalthert added 4 commits November 8, 2017 20:01

Relocate EQ_ASSIGN within the nested parse table

7b8300b

Adapting trees of old tests.

3ffe060

Adding tests for equal assignment

5aaec50

rcmdcheck

ff33ff5

lorenzwalthert requested a review from krlmlr November 8, 2017 19:40

lorenzwalthert mentioned this pull request Nov 8, 2017

if-else with '=' assignment, wrong curly placement #259

Closed

krlmlr approved these changes Nov 8, 2017

View reviewed changes

krlmlr reviewed Nov 8, 2017

View reviewed changes

simplifying cumsum(diff(x), ...), typo, explicit return value

d7238c8

krlmlr reviewed Nov 8, 2017

View reviewed changes

lorenzwalthert force-pushed the fix-eq_assign_parsing branch from 26fd015 to 245c483 Compare November 10, 2017 22:08

consistency

c51460b

lorenzwalthert force-pushed the fix-eq_assign_parsing branch from 245c483 to c51460b Compare November 10, 2017 22:17

lorenzwalthert merged commit 72bc092 into r-lib:master Nov 10, 2017

lorenzwalthert deleted the fix-eq_assign_parsing branch November 10, 2017 22:34

lorenzwalthert added a commit to lorenzwalthert/styler that referenced this pull request Nov 21, 2017

With r-lib#276, we can now indent EQ_ASSIGN correclty.

c24ee59

lorenzwalthert added a commit to lorenzwalthert/styler that referenced this pull request Nov 21, 2017

With r-lib#276, we can now indent EQ_ASSIGN correclty.

e00f0a7

lorenzwalthert mentioned this pull request Jan 23, 2018

Cascading assignment with "=" causes unclear error and can't convert to "<-" #327

Closed

Fix eq assign parsing #276

Fix eq assign parsing #276

Uh oh!

Conversation

lorenzwalthert commented Nov 8, 2017

Uh oh!

codecov-io commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

lorenzwalthert Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorenzwalthert Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

codecov-io commented Nov 8, 2017 •

edited

Loading

lorenzwalthert Nov 8, 2017 •

edited

Loading

lorenzwalthert Nov 8, 2017 •

edited

Loading