Bioconductor / BiocCheck

http://bioconductor.org/packages/BiocCheck
8 stars 26 forks source link

Usage of 'sapply()' and '1:length()' become a warning instead of a NOTE. #91

Closed nturaga closed 2 years ago

nturaga commented 4 years ago
mtmorgan commented 4 years ago

If this is to be a warning then we must be confident that there are no false positives, because people take 'heroic actions' to overcome warnings. Have you run this check (not the whole BiocCheck...) over common Bioconductor packages to get a sense for whether there are false positives?

Also, are there situations where there is nothing or very little to be gained by the change? for instance any(sapply(x, is, "Rle")) evaluates correctly when length(x) == 0 and sapply() returns a list instead of the expected logical vector...

Sometimes when people have corrected 1:length(x) they have written this as seq_len(length(x)) instead of seq_along(x); maybe it makes sense to add some detail in the vignette https://github.com/Bioconductor/BiocCheck/blob/4687a90efd5a30218320af9eb79f778463397d2e/vignettes/BiocCheck.Rmd#L382

grimbough commented 4 years ago

Sometimes when people have corrected 1:length(x) they have written this as seq_len(length(x)) instead of seq_along(x); maybe it makes sense to add some detail in the vignette

Just thought I'd add that I see this all the time in our groups' internal code reviews.

nturaga commented 4 years ago

Thanks for the feedback @mtmorgan. I will check for false positives and the other aspects and get back to you with some details.

lshep commented 4 years ago

@nturaga It looks like you are still working on the updates for checking for false positives and other aspects ?