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

Add 'list' class to list_of vctrs #593

Closed
wants to merge 2 commits into from
Closed

Add 'list' class to list_of vctrs #593

wants to merge 2 commits into from

Conversation

@jeroen
Copy link
Member

jeroen commented Sep 25, 2019

This adds the list class to list_of vctrs. This solves the issue with jsonlite (and possibly other packages implementing S4 list methods):

Perhaps jsonlite is ill-behaved, but I think adding the list class to list_of makes sense?

jeroen added 2 commits Sep 25, 2019
@jeroen jeroen requested a review from hadley Sep 25, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 25, 2019

Codecov Report

Merging #593 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #593      +/-   ##
=========================================
+ Coverage   93.58%   93.6%   +0.02%     
=========================================
  Files          74      74              
  Lines        5630    5633       +3     
=========================================
+ Hits         5269    5273       +4     
+ Misses        361     360       -1
Impacted Files Coverage Δ
R/type-list-of.R 96.72% <100%> (+0.11%) ⬆️
src/utils.c 81.89% <100%> (+0.03%) ⬆️
R/type-bare.R 93.75% <0%> (+1.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da5db68...6b1d634. Read the comment docs.

new_vctr(x, ..., ptype = ptype, class = c(class, "vctrs_list_of"))
# Append 'list' class after 'vctrs_vctr'
out <- new_vctr(x, ..., ptype = ptype, class = c(class, "vctrs_list_of"))
class(out) <- c(class(out), "list")

This comment has been minimized.

Copy link
@hadley

hadley Sep 25, 2019

Member

I think you can just inline list into the class list — the point is not to preserve the existing class, but to lift the base type (which becomes the implicit class) into the subclass.

I'm still not sure this is correct but I'm thinking about it.

This comment has been minimized.

Copy link
@jeroen

jeroen Sep 25, 2019

Author Member

Such that the resulting list_of class will be c("vctrs_list_of", "list", "vctrs_vctr") ?

This comment has been minimized.

Copy link
@lionel-

lionel- Sep 25, 2019

Member

I think "list" should be last because we can't superclass foreign classes.

This comment has been minimized.

Copy link
@jeroen

jeroen Sep 25, 2019

Author Member

Right, that's why I implemented it this way. So keep the PR as is?

This comment has been minimized.

Copy link
@hadley

hadley Sep 25, 2019

Member

I wonder if we need another argument to new_vctr()? Like new_vctr(x, ..., ptype = ptype, class = c(class, "vctrs_list_of"), preserve_implict_class = TRUE) (but a better name)

This comment has been minimized.

Copy link
@hadley

hadley Sep 25, 2019

Member

Another option would be include_type_in_class (that's too long, but might suggest other ideas)

This comment has been minimized.

Copy link
@DavisVaughan

DavisVaughan Sep 25, 2019

Collaborator

explicit_typeof might also work, depending on how we'd extract the base type (just to not be confusing with our other definitions of type/ptype)

This comment has been minimized.

Copy link
@jeroen

jeroen Sep 27, 2019

Author Member

Should I try to add something like this to the PR? Anyone with strong opinions want to take it from here?

This comment has been minimized.

Copy link
@lionel-

lionel- Sep 27, 2019

Member

No worries @jeroen, I can finish the PR when I get back to work on vctrs.

This comment has been minimized.

Copy link
@hadley

hadley Oct 7, 2019

Member

How about extends_type?

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Sep 25, 2019

Ok, @lionel- has convinced me by saying that we could treat the presence of the list class as a signal that the object is a vector (not a scalar, like e.g. lm()). (@DavisVaughan — I think this is how we'll resolve the unnest problem in a user friendly way)

Can you please simplify the test to remove the dependency on a json? I think testing that list_of inherits from list should be adequate.

@DavisVaughan

This comment has been minimized.

Copy link
Collaborator

DavisVaughan commented Sep 25, 2019

I agree with this! This is what I was trying to say in the unnest issue as well

@lionel-

This comment has been minimized.

Copy link
Member

lionel- commented Sep 25, 2019

What is the unnest problem?

@DavisVaughan

This comment has been minimized.

Copy link
Collaborator

DavisVaughan commented Sep 25, 2019

@lionel-

This comment has been minimized.

Copy link
Member

lionel- commented Sep 25, 2019

I like this approach because it gives an easier way of making a class compatible with vctrs. Currently you can declare a list to be a vector by defining a proxy, but then vctrs assumes the class is fully implemented with vctrs primitives and no longer falls back to base methods, eg. to [ when slicing. Adding an explicit "list" class for compatibility with vctrs should be an easy change and undisruptive in most cases.

hadley added a commit that referenced this pull request Nov 11, 2019
Closes #593

Currently failing due to #497
@hadley hadley closed this in #654 Nov 13, 2019
hadley added a commit that referenced this pull request Nov 13, 2019
And add "list" to superclasses of list_of

Closes #593
@jeroen jeroen deleted the listoflist branch Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.