Indent between comment and line break #814
Conversation
Codecov Report
@@ Coverage Diff @@
## master #814 +/- ##
==========================================
+ Coverage 90.45% 90.47% +0.01%
==========================================
Files 47 47
Lines 2430 2435 +5
==========================================
+ Hits 2198 2203 +5
Misses 232 232
Continue to review full report at Codecov.
|
|
This is how benchmark results would change (along with a 95% confience interval in relative change) if ee0ade9 is merged into master:
Further explanation regarding interpretation and methodology can be found in the documentation. |
|
This is how benchmark results would change (along with a 95% confience interval in relative change) if d07b7ea is merged into master:
Further explanation regarding interpretation and methodology can be found in the documentation. |
| @@ -54,12 +54,21 @@ indent_op <- function(pd, | |||
| indent_eq_sub <- function(pd, | |||
| indent_by, | |||
| token = c("EQ_SUB", "EQ_FORMALS")) { | |||
| eq_sub <- which(pd$token %in% token) | |||
There was a problem hiding this comment.
@MichaelChirico changing to boolean here because I think it's slightly more efficient, but later realised we need to convert to int again for handling your exception
There was a problem hiding this comment.
Thanks for fixing. How did you know to look in indent_eq_sub if I might ask?
There was a problem hiding this comment.
Oh I just saw your comment on the issue, nvm.
There was a problem hiding this comment.
I just full text searched the rules in R/rules-indention.R With “eq_sub” to find a rule that has this in the name.
| indent_indices <- intersect(eq_sub + 1, has_line_break) | ||
| has_line_break <- pd$lag_newlines > 0 | pd$token == "COMMENT" | ||
| indent_indices <- which(lag(eq_sub, default = FALSE) & has_line_break) | ||
| if (any(pd$token[indent_indices] == "COMMENT")) { |
There was a problem hiding this comment.
As the case in #813 is probably occurring quite infrequently, avoid an unnecessary map() call for performance reasons.
There was a problem hiding this comment.
Wouldn't a subassign be faster?
comment_idx <- pd$token[indent_indices] == "COMMENT"
indent_indices[comment_idx] <- next_non_comment(pd, indent_indices[comment_idx])
(or a map on the RHS of <- if next_non_comment isn't vectorized in arg 2)
There was a problem hiding this comment.
It’s not vectorised. Good thing now with {touchstone} we can see what’s faster for an average Code input. I can try that. Or we can benchmark just this changed expression locally.
There was a problem hiding this comment.
apparently not, sub-assignment and sub-setting is also expensive. All in all it's about the same.
# devtools::load_all()
# debug(indent_eq_sub)
# run some code
bench::mark(
first = {
indent_indices <- which(lag(eq_sub, default = FALSE) & has_line_break)
x <- purrr::map_int(indent_indices, function(idx) {
if (pd$token[idx] == "COMMENT") {
next_non_comment(pd, idx)
} else {
idx
}
})
x
},
second = {
indent_indices <- which(lag(eq_sub, default = FALSE) & has_line_break)
comment_idx <- pd$token[indent_indices] == "COMMENT"
indent_indices[comment_idx] <- purrr::map_int(indent_indices[comment_idx], function(idx) {
next_non_comment(pd, idx)
})
indent_indices
}
)
#> # A tibble: 2 x 13
#> expression min median `itr/sec` mem_alloc `gc/sec` n_itr n_gc total_time result memory time gc
#> <bch:expr> <bch:t> <bch:t> <dbl> <bch:byt> <dbl> <int> <dbl> <bch:tm> <list> <list> <list> <list>
#> 1 first 88µs 95.4µs 8973. 0B 2.03 4420 1 493ms <int … <Rprof… <benc… <tibb…
#> 2 second 89.8µs 95.2µs 9172. 0B 4.26 4310 2 470ms <int … <Rprof… <benc… <tibb…
|
@MichaelChirico if you have suggestions for improvement, happy to hear them 😀. I don't think it's very elegant. |
Closes #813.