-
Notifications
You must be signed in to change notification settings - Fork 76
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
Adds trust parameter with deprecation for default #409
Adds trust parameter with deprecation for default #409
Conversation
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.
Meta comments:
- The
trust
parameter should be mentioned in the documentation. Probably don't need to make it a@param
. Maybe a new section ontrust
would be nice. - Update
NEWS.md
@jsonbecker Added my several comments. Please consider them. Thank you very much! |
These are great suggestions I'll probably work on today. I mostly wanted to get up a proof of the idea after discussing it in the issue. But with the improvements you proposed, I feel pretty good about this as an idea. |
if (isFALSE(trust)) { | ||
stop(format, "files may execute arbitary code. Only load", format, "files that you personally generated or can trust the origin.", call. = FALSE) | ||
} | ||
NULL |
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.
I used this from review example provided, but curious if an explicit NULL
return is necessary/best practice. This would be a bit new to me.
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.
You can choose
is.null((function() { NULL })())
#> [1] TRUE
is.null((function() { })())
#> [1] TRUE
is.null((function() { return() })())
#> [1] TRUE
is.null((function() { invisible() })())
#> [1] TRUE
Created on 2024-05-12 with reprex v2.1.0
load(file = file, envir = envir) | ||
if (missing(which)) { | ||
if (length(ls(envir)) > 1) { | ||
warning("Rdata file contains multiple objects. Returning first object.") |
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.
This is a minor formatting point (4 SPCs). I can fix it later.
@jsonbecker I will merge this now. But I will also make some slight adjustments (mostly on the documentation in the |
* Documentation (in import() as well as not using serialization formats in the overview vignette) * Correct the deprecating warning * Add an undocumented option: rio.import.trust. We don't need to advertise this to encourage users to override this.
This is an attempt to address #406 by adding a new parameter
trust
set toTRUE
by default with a deprecation warning that this will flip toFALSE
in2.0.0
.