-
Notifications
You must be signed in to change notification settings - Fork 80
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
Create print.one_skim_df, document reassign_skim_attrs and modify re… #703
base: main
Are you sure you want to change the base?
Conversation
…oncile_skimrs in response to changes in 4.2 and R CMD CHK issues.
As stated in the issue, we were getting an "object of length > 1" warning but not seeing it, now it is an error. We need to do a release ASAP but I'm not sure why this wasn't caught by testing on the develop branch. |
#' Pass attributes from a `skimr` object to a new object. | ||
#' @noRd |
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'm not sure that this change accomplishes anything. For one, I don't see a documentation file generated to match this function. That's a good thing. Documenting this function (removing the noRd tag) is an issue because it is not exported.
Do you mind reverting 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.
That's what is causing one of the errors when it builds.
@@ -61,6 +61,27 @@ print.skim_df <- function(x, | |||
} | |||
} | |||
|
|||
#' @export | |||
print.one_skim_df <- function(x, |
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.
We don't have a summary method for a one_skim_df. Can we remove the logic for the summary and instead have this?
print.one_skim_df <- function(x, n, width, ...) {
NextMethod("print")
}
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 took the logic of
Line 38 in 562b753
print.skim_df <- function(x, |
but took out the purrr.
I'm not sure but now that we are not silently getting the warning it may be that other parts are getting hit in new ways.
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.
Maybe just make include_summary = FALSE
?
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.
Thanks for these last fixes! I just have a couple of comments on the change..
So overall the initial issue was that there was no print.one_skim_df. Then that then started the issue of reconcile_skimmers not working because of the warning now being an error. |
@michaelquinn32 I think I replied to at least some of your comments can you take a look? The main issue is that we were generating a warning and now it's an error. We just didn't realize there was a warning. |
Sorry to keep pushing back, but I'm not sure this is still an issue that needs to be fixed. For example, here is the most recent r-devel check against our main branch. I don't see a warning. I just put together a dev environment on a machine with R 4.2.1, and I didn't see any issues there either. Do you have an example where we have an issue with the current main or develop branch? If you can provide one, I can take another look. Thanks! And I appreciate the patience with all of this. |
…concile_skimrs in response to changes in 4.2 and R CMD CHK issues.
#702