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

Auto-load tibble #154

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@krlmlr
Copy link
Member

krlmlr commented Mar 29, 2016

Fixes r-dbi/RSQLite#82 and others.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented May 30, 2016

Hmmmm, I've held off on doing this in the past because I didn't want to break the usual data frame behaviour unless you explicitly opted-in by loading dplyr/tibble.

@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented May 30, 2016

I'd argue that this is dangerous. If we want opt-in, we should always return a plain data frame and ask users to call as_data_frame(). Otherwise the behavior depends on the load state of a package, which might be not entirely under the control of the user.

How about just declaring this as a potentially breaking change? Users affected by this still can call as.data.frame() on the result.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 30, 2016

Current coverage is 58.66%

Merging #154 into master will decrease coverage by <.01%

@@             master       #154   diff @@
==========================================
  Files            31         31          
  Lines          3663       3665     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           2151       2150     -1   
- Misses         1512       1515     +3   
  Partials          0          0          

Powered by Codecov. Last updated by 1326f00...cf7f370

Kirill Müller

@krlmlr krlmlr force-pushed the krlmlr:feature/tibble branch from aa1640a to b4852c0 May 30, 2016

@hadley

This comment has been minimized.

Copy link
Member

hadley commented May 30, 2016

It's under control enough by the user - if they're using dplyr, they'll understand the behaviour. If they're not, they don't need to care about it. This is a big change that will need to happen with readr, readxl, etc in order to be consistent.

@hadley hadley closed this May 30, 2016

@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented May 31, 2016

Something else might load the tibble package, without the user being aware of it -- tibble already has reverse imports. Loading tibble is enough to change the behavior, that's why I think the current state is dangerous.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Jun 3, 2016

Agreed.

hadley added a commit that referenced this pull request Jun 3, 2016

@krlmlr krlmlr deleted the krlmlr:feature/tibble branch Jun 3, 2016

@lock lock bot locked and limited conversation to collaborators Jun 26, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.