-
-
Notifications
You must be signed in to change notification settings - Fork 996
remove dependency to tibble #853
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
| else if (df_print == "tibble") | ||
| else if (df_print == "tibble") { | ||
| if (!requireNamespace("tibble", quietly = TRUE)) | ||
| stop("Printing 'tibble' without 'tibble' package available") |
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 wonder if instead of making this a hard error we could just use the default print method (and maybe warn once per session)?
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.
@kevinushey how do we usually warn once per session? By setting a global or there is a nicer way for doing this?
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.
A slightly cleaner implementation would be with a factory function e.g.
make_warning <- function(message) {
message <- message
warned <- FALSE
function() {
if (warned) return()
warning(message)
warned <<- TRUE
}
}
tibble_warning <- make_warning("...")
tibble_warning()
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 actually think that if they ask for tibble and we can't provide it then we are just fine to error (I think this will be a very narrow case as tibble is installed with dplyr and won't be a common choice given that pagedtable is available).
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.
kept as is.
R/html_paged.R
Outdated
| is_atomic(x) || is.list(x) | ||
| } | ||
|
|
||
| is_vector_s3 <- function(x) UseMethod("is_vector_s3") |
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.
@kevinushey and @hadley Are we okay to have a package private S3 method here or could that interfere somehow with the tibble exported method of the same name? We could just rename it to is_s3_vector and/or just implement it with a switch/case.
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.
@jjallaire just in case, renamed.
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'd need to experiment to check - S3 + namespaces is still a little hairy. Definitely safer to rename.
f7ac3b8 to
aee6290
Compare
|
Yes, I'd say let's not use S3 here. On Thu, Nov 3, 2016 at 7:20 AM, Hadley Wickham notifications@github.com
|
|
K, S3 removed. |
|
FWIW, I'm planning to support R 3.0.0 in tibble 1.3.0 which will be out soon. |
Consider removing
tibbledependency. Two PR are out for supportingR 3.0.0intibbleandlazyeval:hadley/lazyeval#87
tidyverse/tibble#187
However, we could take the code out from
tibbleif needed as well.