Expose internals used with other style guides #1043
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1043 +/- ##
==========================================
- Coverage 91.06% 91.05% -0.02%
==========================================
Files 46 46
Lines 2687 2682 -5
==========================================
- Hits 2447 2442 -5
Misses 240 240
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 89433fa is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
|
Hi @IndrajeetPatil just following-up on this, is there any way I can help progress? Will be great to be able to have |
| #' pd <- compute_parse_data_nested(code) | ||
| #' is_curly_expr(pd) | ||
| #' is_curly_expr(pd$child$`17`$child$`14`) | ||
| #' |
There was a problem hiding this comment.
The blank lines are deliberate.
Without them, all examples are squished together like a centipede, and it's hard to tell where one ends and the other starts.
| #' is_function_declaration(pd$child$`12`$child$`11`) | ||
| #' | ||
| #' @export | ||
| is_function_declaration <- function(pd) { |
There was a problem hiding this comment.
I think this is a better name than is_function_dec().
| #' is_conditional_expr(pd$child$`24`) | ||
| #' | ||
| #' @export | ||
| is_conditional_expr <- function(pd) { |
There was a problem hiding this comment.
I think this is a better name than is_cond_expr().
There was a problem hiding this comment.
But why do we need to change all the *_expr() functions? Their current names are expressive as they are.
There was a problem hiding this comment.
no sorry I read your comment wrongly (which is why I deleted it). I thought you wanted *_expression() everywhere, but that is (obviously) not the case.
There was a problem hiding this comment.
I see.
Okay, then this should be ready for your review. I don't have any additional changes in mind.
|
Hi @Robinlovelace, I am done on my end. Now I await code review. Once that's done, I think this should be good to go and might even be part of the next release (cf. #1044). |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 11b3264 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
|
That is a great update, thanks. 🤞 for the next release! |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 11b3264 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
|
Thanks for the work on this PR. @IndrajeetPatil I saw the review request. Please do not re-request a review from me. I don't forget about them, but I don't have time to look into it now but I will so in due time. |
this should be more robust against changes in the R parser with the goal to avoid CRAN submission due to failing examples
|
@IndrajeetPatil can you have a look at my last commit (can't request a review from you for some reason)? After this one is approved by you, I will prepare the release as discussed in #1044. |
The one who made PR can't review it themselves. That's why you can't assign me to review my own PR.
Your changes look good to me. Thanks. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 290a73a is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
Re-implementation of abandoned #1037, which had complex merge conflicts.
Closes #974.
Progress tracker
compute_parse_data_nested()helperis_*()helpersUpdate NEWS({fledge}is supposed to take care of this)