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

Implement DataValueInterfaces definitions #57

Merged
merged 6 commits into from
Jul 2, 2019
Merged

Implement DataValueInterfaces definitions #57

merged 6 commits into from
Jul 2, 2019

Conversation

quinnj
Copy link
Contributor

@quinnj quinnj commented May 8, 2019

No description provided.

@quinnj
Copy link
Contributor Author

quinnj commented May 23, 2019

Bump

@davidanthoff
Copy link
Member

I'm completely swamped through the end of next week and won't be able to engage on this until then. I'll try to follow up then, but if I forget, feel free to bump again :)

@quinnj
Copy link
Contributor Author

quinnj commented Jun 10, 2019

Bump again

@davidanthoff
Copy link
Member

I'll have some time this week to look at this!

@quinnj quinnj changed the title Implement DataAPI interface Implement AbstractDataValues interface Jun 27, 2019
@quinnj quinnj changed the title Implement AbstractDataValues interface Implement AbstractDataValues definitions Jun 27, 2019
@quinnj
Copy link
Contributor Author

quinnj commented Jun 27, 2019

@davidanthoff, I've updated this PR to point to https://github.com/JuliaData/AbstractDataValues.jl (happy to move the package to Queryverse if you'd like; I'm not sure it really matters where it lives, as long as we both have admin access).

One thing I realized from what we talked about: having DataValues overload Base.nonmissingtype doesn't quite achieve the same purpose as nondatavaluetype because we're really looking for the Union{T, Missing} equivalent of DataValue{T} (and vice-versa for datavaluetype), which conflicts with the purpose of Base.nonmissingtype, which is to get the T from Union{T, Missing} (so the equivalent DataValue definition would be Base.nonmissingtype(::Type{DataValue{T}}) where {T} = T). I also left unwrap in for now as I haven't tested moving things over to using convert. I'll give that a shot and report back if we can remove it, but wanted to ping you on the initial setup here first.

@davidanthoff
Copy link
Member

Cool!

I think it might make slightly more sense to host the package in queryverse? That way the two DataValue packages would be close together. But I also don't feel strongly about it. Oh, the one (really not very important) benefit would be that it would show up at https://www.queryverse.org/status/.

Should we maybe call the package DataValuesInterface.jl? Abstract to me implies a type, which we don't really have in that package?

re nondatavaluetype, yep, I see your point. I have two unrelated thoughts on that: a) should it be called missingtype, to be symmetric with datavaluetype? Maybe that is slightly more specific, by telling a user what they will get? b) I wonder whether this could still be folded into something existing, though... Maybe something like convert(Union{nonmissingtype(typeof(x)), Missing}, typeof(x)? But that is probably an abuse of convert... And maybe we should also not let the perfect be the enemy of the working in this case...

Re unwrap, yes, lets see whether you can get rid of that somehow. There is also x[] that unwraps, not sure whether that might also work?

One other random thought: the whole exercise does start to feel a little like defining an interface for different missing types... Probably not where we want to go?

@quinnj
Copy link
Contributor Author

quinnj commented Jun 27, 2019

Sounds good. Could you create the repo in Queryverse org then and then add me as (at least) a collaborator? Then I can do the initial setup and update the PR here.

@quinnj quinnj changed the title Implement AbstractDataValues definitions Implement DataValueInterfaces definitions Jun 28, 2019
@quinnj
Copy link
Contributor Author

quinnj commented Jun 28, 2019

Ok, I've pushed all the code for new DataValueInterfaces.jl: https://github.com/queryverse/DataValueInterfaces.jl. Tables.jl PR is here. I'll add some tests here.

@davidanthoff
Copy link
Member

Looks good. I probably have to trigger the registration of DataValueInterfaces? Let me know when you are ready with all the rest, and then I'll trigger the registration, and then we can merge this here.

@quinnj
Copy link
Contributor Author

quinnj commented Jun 28, 2019

I think we're ready; let's trigger DataValueInterfaces.jl registration and I'll do a little more stress-testing on Tables.jl in the mean time.

@davidanthoff
Copy link
Member

@davidanthoff davidanthoff reopened this Jul 1, 2019
@davidanthoff
Copy link
Member

@quinnj I just restarted tests, and I think they are missing a using DataValueInterfaces?

@quinnj
Copy link
Contributor Author

quinnj commented Jul 2, 2019

Alright, let's see how this goes (everything passes on my machine)

@codecov-io
Copy link

codecov-io commented Jul 2, 2019

Codecov Report

Merging #57 into master will decrease coverage by 0.09%.
The diff coverage is 66.66%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #57     +/-   ##
========================================
- Coverage    74.7%   74.6%   -0.1%     
========================================
  Files          13      13             
  Lines         506     512      +6     
========================================
+ Hits          378     382      +4     
- Misses        128     130      +2
Impacted Files Coverage Δ
src/scalar/core.jl 92.59% <66.66%> (-1.21%) ⬇️

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 b6b0735...8b927d5. Read the comment docs.

@quinnj
Copy link
Contributor Author

quinnj commented Jul 2, 2019

Ok, tests look good here. Once we merge and tag a release here, I can re-run tests here and merge/tag there. Getting close!

@davidanthoff davidanthoff merged commit 4a023f9 into queryverse:master Jul 2, 2019
@davidanthoff
Copy link
Member

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.

None yet

3 participants