use newer syntax #1031
use newer syntax #1031
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes R syntax throughout the codebase by adopting newer language features introduced in R 4.1.0+ and contemporary dplyr patterns.
Key Changes:
- Replaced
function(x)with anonymous function shorthand\(x)syntax - Modernized dplyr code from
group_by() %>% tally() %>% ungroup()tosummarise(.by = ...)pattern - Replaced
isFALSE()with!operator for logical negation - Changed
ifelse()toif-elsefor scalar conditional assignments - Updated GitHub Actions workflow for code coverage with better configuration
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| R/utils.R | Updated error handler to use anonymous function shorthand \(e) |
| R/ggpiestats.R | Changed isFALSE(paired) to !paired in condition |
| R/ggpiestats-ggbarstats-helpers.R | Modernized .cat_counter to use .by parameter; updated .chisq_test_safe with new syntax and parameter handling |
| R/gghistostats.R | Updated labels parameter to use anonymous function shorthand |
| R/ggbetweenstats-helpers.R | Changed .f_switch from vectorized ifelse() to scalar if-else |
| R/ggbarstats.R | Changed isFALSE(paired) to !paired in condition |
| R/extract-stats.R | Converted multiple function definitions to anonymous function shorthand |
| .github/workflows/test-coverage.yaml | Enhanced workflow with explicit permissions, updated runner, improved coverage reporting, and artifact upload |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ungroup() %>% | ||
| arrange(desc({{ x }})) %>% | ||
| summarise(counts = n(), .by = c({{ y }}, {{ x }})) %>% | ||
| mutate(perc = (counts / sum(counts)) * 100, .by = {{ y }}) %>% |
There was a problem hiding this comment.
The percentage calculation logic has changed. In the old code, percentages were calculated across all rows (after ungroup). In the new code, .by = {{ y }} causes percentages to be calculated within each y group. This is a semantic change that could affect the results.
If y is NULL (as in line 43 where .cat_counter(data, {{ y }}) is called with only one argument), the behavior might be the same, but when y is not NULL (as in line 12 where .cat_counter(data, {{ x }}, {{ y }}) is called), the percentages will be calculated per y group instead of overall.
To maintain the original behavior, remove the .by parameter from the mutate call.
| arrange(desc({{ x }})) %>% | ||
| summarise(counts = n(), .by = c({{ y }}, {{ x }})) %>% | ||
| mutate(perc = (counts / sum(counts)) * 100, .by = {{ y }}) %>% | ||
| arrange(desc({{ x }}), {{ y }}) %>% |
There was a problem hiding this comment.
The sorting behavior has changed. The old code only sorted by desc({{ x }}), but the new code sorts by both desc({{ x }}) and {{ y }}. This adds a secondary sort criterion that wasn't present before and could change the order of rows in the output, potentially affecting downstream code that relies on the specific ordering.
No description provided.