Skip to content
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

Use tibble package #1595

Closed
wants to merge 16 commits into from
Closed

Use tibble package #1595

wants to merge 16 commits into from

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Dec 23, 2015

Reexports all functions exported by tibble.

Fixes #1488.

A few tests (of functions now private to tibble) had to be removed. One test had to be adapted, because names(data_frame()) is now character(0L) instead of NULL.

@krlmlr
Copy link
Member Author

krlmlr commented Mar 1, 2016

Updated, now leaves src() and tbl() related functions in this package; compatible with tibble 0.1-5.

@@ -38,21 +38,21 @@ test_that("add_rownames keeps the tbl classes (#882)", {

test_that("2d object isn't a valid column", {
expect_error(
check_data_frame(list(x = mtcars)),
tibble:::check_data_frame(list(x = mtcars)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe these tests should just be deleted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes -- keeping them for now to avoid merge conflicts with further incoming changes.

@krlmlr krlmlr force-pushed the tibble-2 branch 3 times, most recently from 9829af7 to 5cabaf2 Compare March 8, 2016 21:27
@krlmlr krlmlr mentioned this pull request Mar 9, 2016
3 tasks
@hadley
Copy link
Member

hadley commented Mar 10, 2016

Could you please merge/rebase? I think I'm ready to start seriously looking at this.

We also need to move tests and move anything data_frame() related in the tibble news.

@krlmlr
Copy link
Member Author

krlmlr commented Mar 10, 2016

Done. I think tests may stay in both places for now; tibble test coverage is good already, but keeping the tests in dplyr makes sure that the original functionality remains.

@krlmlr
Copy link
Member Author

krlmlr commented Mar 21, 2016

Rebased, deprecated add_rownames(), re-added dim_desc(). Had to adapt test due to changed semantics of type_sum() for data frames. Tests pass locally.

Will remove the "Remotes" entry once tibble is on CRAN. Otherwise ready for review.

- expect deprecation warning

#' @importFrom tibble tbl_df
#' @export
tibble::tbl_df
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit late now, but I realised that tbl_df probably should've stayed in dplyr. It's not a big deal though, as tbl_df() is basically deprecated in favour of as_data_frame()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No big deal, I can make a point release.

@hadley
Copy link
Member

hadley commented Mar 24, 2016

I think this looks good. Can you please add a bullet to NEWS?

What do you think about removing the tibble related tests now that tibble has 100% test coverage?

@hadley
Copy link
Member

hadley commented Mar 24, 2016

Actually don't worry about NEWS - I'm starting to do the pre-release cleanup

@hadley hadley closed this in #1737 Mar 24, 2016
@krlmlr krlmlr deleted the tibble-2 branch May 9, 2016 09:19
@lock
Copy link

lock bot commented Jan 19, 2019

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants