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

Fix Union{} corner case in isiterabletable #21

Merged
merged 3 commits into from
Apr 1, 2021

Conversation

bkamins
Copy link
Contributor

@bkamins bkamins commented Sep 16, 2020

This is a corner case as Union{} <: NamedTuple but I guess you do not want to support Union{} eltype to be true in this test?

This is a corner case as `Union{} <: NamedTuple` but I guess you do not want to support `Union{}` eltype to be `true` in this test?
@bkamins
Copy link
Contributor Author

bkamins commented Sep 20, 2020

@davidanthoff - is this OK for you?

@bkamins
Copy link
Contributor Author

bkamins commented Sep 29, 2020

@davidanthoff - is is OK to be merged?

@davidanthoff
Copy link
Member

I think I don't fully understand how we could end up in this situation. Does it make sense for something to return HasEltype() and then return Union{} as the element type? There are no values that are of type Union{}, right? So wouldn't it make more sense for a source to just say it doesn't have an element type in that case?

@bkamins
Copy link
Contributor Author

bkamins commented Sep 30, 2020

See the linked issue in Tables.jl for details. The offending test is for Matrix{Union{}}(undef, 2, 3) and it is allowed in Base (not useful, but valid, and it is tested against in Tables.jl).

@bkamins
Copy link
Contributor Author

bkamins commented Oct 1, 2020

It would be great if it could be merged (or rejected) and if it is merged - a new release of TableTraits.jl was tagged.
The reason is that we need a change in DataFrames.jl which requires a change (and release) in Tables.jl, which in turn requires a decision in this PR.

Thank you!

@bkamins
Copy link
Contributor Author

bkamins commented Oct 24, 2020

Is there any decision on this issue?

@davidanthoff
Copy link
Member

Sorry that this took so long, I had to just give up on certain topics in the fall, too many things going on at work :) I'm merging this as soon as tests pass and will then tag a patch release.

@davidanthoff davidanthoff merged commit 2106bc6 into queryverse:master Apr 1, 2021
@bkamins bkamins deleted the patch-1 branch April 1, 2021 22:27
@bkamins
Copy link
Contributor Author

bkamins commented Apr 1, 2021

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants