-
Notifications
You must be signed in to change notification settings - Fork 19
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
length
and iteration?
#39
Comments
I think for One way to test some of these out would be to take @nalimilan's branch to port DataArray's to Nulls and see what tests fail: JuliaStats/DataArrays.jl#288 |
Let's remove iteration on More seriously, I agree we should try removing these methods and only reintroduce them if we realize they are really needed. Testing DataArrays first is a good idea, I'll do that after updating it to take into account recent changes in Nulls.jl. |
Just tried it, there are only a few lines to change. Three of them explicitly tested the removed methods, so that's expected. Two others are of the form: dvstr = @data ["one", "two", null, "four"]
all([length(x)::Int for x in dvstr] == [3, 3, 1, 4]) I think they also qualify as non-use cases. So overall I think we should remove them. We could also imagine providing |
For clarity, the full set of methods I'm talking about is: length, size, ndims, getindex, start, next, done |
I did? o_O I think that |
See #40. |
I think this package is going very well and I'm on board with most of it (e.g. the behavior of comparisons and logical operators), but I find the definitions of
length
, iteration, and the array interface methods pretty sketchy. Those would be OK ifnull
always represented a missing number, but if it's going to represent a missing string (or something else) then those will give misleading results. Are we 100% sure we need those methods? The approach I favor is to add no methods toNull
until it becomes extremely clear that they're needed to avoid major pain. Are there examples where that threshold has been reached for e.g.length
?The text was updated successfully, but these errors were encountered: