-
Notifications
You must be signed in to change notification settings - Fork 195
Add a third fuzzer for @/$ equivalency #2821
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2821 +/- ##
=======================================
Coverage 99.27% 99.27%
=======================================
Files 128 128
Lines 7165 7188 +23
=======================================
+ Hits 7113 7136 +23
Misses 52 52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
R/source_utils.R
Outdated
| } else { | ||
| res <- function_call_cache[names(function_call_cache) %in% function_names] | ||
| } | ||
| if (include_s4_slots) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have 4 branches anyway, we might as well make them parallel, WDYT?
if (!is.null(function_names)) {
if (include_s4_slots) {
res <- combine_nodesets(function_call_cache, s4_slot_cache)
} else {
res <- function_call_cache
}
} else {
if (include_s4_slots) {
res <- combine_nodesets(
function_call_cache[names(function_call_cache) %in% function_names],
s4_slot_cache[names(s4_slot_cache) %in% function_names]
)
} else {
res <- function_call_cache[names(function_call_cache) %in% function_names]
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me! The only downside I saw was repeating the clutter-ish names(function_call_cache) %in% function_names, so I pulled that into a variable. I think that also saves us the "embarassment" of having to disable unnecessary_nesting_linter() which is not equipped to allow "highly parallel" if/else structures like that.
Part of #2191 -- continuing #2819. Also progress on #2737.
This one caught a pretty big gap in our current linter logic, as noted in #2820. Those are addressed here.
I left the new
include_s4_slots=FALSEby default since apparentlyTRUEis only needed in one place to pass the current fuzz suite.