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

Support an empty table with Union{} rows #56

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andyferris
Copy link
Member

Fixes #55.

@tkf I was wondering if this is the kind of thing you would like to push!! to? I guess it would be nice to see a prototype working.

@codecov-io
Copy link

codecov-io commented Nov 7, 2019

Codecov Report

Merging #56 into master will decrease coverage by 0.86%.
The diff coverage is 12.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #56      +/-   ##
=========================================
- Coverage   67.06%   66.2%   -0.87%     
=========================================
  Files           6       6              
  Lines         498     506       +8     
=========================================
+ Hits          334     335       +1     
- Misses        164     171       +7
Impacted Files Coverage Δ
src/Table.jl 71.69% <0%> (-4.31%) ⬇️
src/show.jl 50% <50%> (ø) ⬆️

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 fc1247a...48e3c69. Read the comment docs.


# Specialize for empty table of type `Union{}`
const EmptyTable{N} = Table{Union{}, N, Nothing}
Base.empty(::Table{<:Any, 1}, ::Type{Union{}}) = EmptyTable{1}(nothing)
Copy link

Choose a reason for hiding this comment

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

What I had in mind was something like

Suggested change
Base.empty(::Table{<:Any, 1}, ::Type{Union{}}) = EmptyTable{1}(nothing)
Base.empty(::Type{<:Table{<:Any, 1}}) = EmptyTable{1}(nothing)

i.e., I want an empty table given its type. I also would like to suggest omitting the eltype since this is the only eltype that is guaranteed to be able to widen to anything.

Copy link
Member Author

@andyferris andyferris Nov 9, 2019

Choose a reason for hiding this comment

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

Oh you want to use types not instances? I’ve been heavily trending the other direction lately.

And the last part - you mean omit the eltype in empty’s function signature and default to Union{}, not the eltype of the input?

Copy link

Choose a reason for hiding this comment

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

For the first part, please see my comment in #55 (comment)

you mean omit the eltype in empty’s function signature and default to Union{}, not the eltype of the input?

I was suggesting to make the second argument optional. So, e.g., returning Union{}[] from empty(::Type{Vector}). Though empty(::Type{Vector{Float64}}) probably still should return Float64[].

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.

Table() throws - implement zero column tables
3 participants