-
Notifications
You must be signed in to change notification settings - Fork 45
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
closes #784 by fixing typename + tests #786
Conversation
Co-authored-by: Okon Samuel <[email protected]>
Co-authored-by: Okon Samuel <[email protected]>
Co-authored-by: Okon Samuel <[email protected]>
thanks for the review @OkonSamuel feel free to take this over, I'm not close enough to the Tables API to know what's best so I trust your choices are better, it might be good to check the tests are right, maybe do a side test with what brought Jack_N to open the issue in the first place and check that there's no bottleneck anymore? |
Thank you both for helping with the changes. Also, it may be a good idea to make the slight modification to the selectcols function so that the '!' is not needed in the if statement. The code is below so you can see what I mean. Removing the unnecessary double negative should help keep the code clean and understandable.
|
Codecov Report
@@ Coverage Diff @@
## dev #786 +/- ##
=======================================
Coverage 85.92% 85.93%
=======================================
Files 36 36
Lines 3460 3462 +2
=======================================
+ Hits 2973 2975 +2
Misses 487 487
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OkonSamuel I'm happy with this, modulo my single suggestion. Please merge and tag a new release at your leisure.
I just merged (it was my branch so not sure if sam could do it) will leave the tag & release to you guys |
Thank you @tlienart and @OkonSamuel . A new release just tagged. |
typename
forDataFrames.DataFrame
object, tested with1.0
to1.7
seems okDataFrames
to theextras
)Note: this
isdataframe
stuff should eventually be removed via improvements to Tables.jl (see here) but for now is necessary to avoid big bottlenecks.