Use a slightly altered isS3stdGeneric() to detect non-standard generics
#857
Conversation
|
@jimhester it appears that |
|
Added a bullet to News |
|
Is there anything else I can do to get this past the finish line? I'm happy to rebase/unconflict NEWS if that helps. |
|
This PR should be compared using the branch compare script to make sure all changes in linting behavior are intended compared to master. If you want, you can try running the comparison script found in |
|
I did run the branch compare script on the first version of the implementation (the only additions after that were NEWS + making it use the new code in all versions of R not just those > 3.5)
Was what I ran sufficient? Is there a better way to run or report the results from those? Happy to learn what the appropriate way to run/report those are if I've made a mistake. I can also run it again if that's helpful (though I'm running R 4.1 or 4.0 locally, so the code change shouldn't actually do anything differently). |
|
It would be nice to see a sample of the lints that disappear from the checked packages in comparison to master to ensure no false negatives occur as well as to verify that no new lints appear (which I think you did). |
MichaelChirico
force-pushed
the
generics-detection-846
branch
from
f4160cb
to
251e174
Compare
|
@AshesITR I don't see a lot of risk here for unexpected regression, so going ahead to merge. Part of my release to-do list is to do some more extensive compare_branches runs of CRAN vs. current dev, so any concerns should be addressed by that as well. |
|
Okay with me as long as we do a before/after comparison at some point in time before release. |
| func <- function(x) { | ||
| print(x) | ||
| UseMethod("func") | ||
| } |
There was a problem hiding this comment.
Is there a valid use case for the other way around?
i.e. for
func <- function(x) {
x <- UseMethod("func")
common_postprocessing(x)
}
There was a problem hiding this comment.
let's try & keep close to the base definition unless there's a compelling reason? i'm also a bit hesitant for this change if it's not being suggested upstream as well
There was a problem hiding this comment.
The base definition checks the first call, so would find my example iinm. That's why I'm asking.
There was a problem hiding this comment.
isn't the call that's found <- though, not UseMethod?
There was a problem hiding this comment.
Ah, right. I wonder why base R looks for the first call instead of the last, then...
There was a problem hiding this comment.
I didn't see anything about isS3stdGeneric() on r-devel in a quick search... maybe worth bringing up there.
cc @moodymudskipper who identified the bug fix at root in #1161
I followed the path described in #846 and added a test. If it's preferable I'm happy to make that test be more of an integration-style test with one of the test fixture packages in the
testsdirectory. I decided to not (yet?) add functionality to scan forUseMethod()elsewhere in the function, since that could produce false positives (if it's in an if/else and the other branch doesn't callUseMethod(), though that would be a very odd use ofUseMethod()IMO). I'm happy to add it in if y'all think that's useful (or add a TODO that one could do that if there are requests to). I also still gated this on R > 3.5.0, though if we've implemented this ourselves and aren't depending onutils, we could probably use that for any version of R.I also ran
.dev/compare_branches.R(after a bit of RTFS to figure out how to call it). I couldn't find any documentation for the right way to interpret the results, but what I did was look at the output and confirm that there were no new lints in the"branch"rows that aren't also there in the"main"rows. Is there anything else I should do to use that script?Resolves #846