-
Notifications
You must be signed in to change notification settings - Fork 188
Support case where specified method ignores mode in download_linter() #2883
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2883 +/- ##
=======================================
Coverage 99.27% 99.27%
=======================================
Files 128 128
Lines 7143 7160 +17
=======================================
+ Hits 7091 7108 +17
Misses 52 52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -40,14 +40,36 @@ download_file_linter <- function() { | |||
|
|||
implicit_mode <- is.na(download_file_mode) | |||
|
|||
# According to download.file() documentation, mode is ignored when method |
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.
include a corresponding example?
R/download_file_linter.R
Outdated
following-sibling::expr[1]/STR_CONST | ||
" | ||
) | ||
no_mode_method <- !is.na(download_file_method) & download_file_method %in% c("wget", "curl") |
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.
naming suggestion: has_ignored_mode
or method_ignores_mode
. Particularly would like to avoid !no
double-negation
# mode explicitly specified with a method that ignores it | ||
lint_message <- rex::rex("mode argument value is ignored") | ||
expect_lint("download.file(x, method = 'wget', mode = 'w')", lint_message, linter) | ||
expect_lint("download.file(x, method = 'curl', mode = 'wb')", lint_message, linter) |
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.
add a test where method=method
, i.e., given as a name
not a character literal, as well as an explicit setting outside these, e.g. method="internal"
.
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.
The gist LGTM, feel free to merge after addressing the minor feedback. Thanks!
Follow up from #2882