RFC: work around MethodErrors with Base.OneTo #205
Conversation
to work around OneTo issue
tkelman
changed the title
on output of broadcast_shape and index_shape still need to work out a checkbounds test
|
Thanks. Looks good to me, though I'd have called these |
tkelman
changed the title
|
Unfortunately it seems to break an |
Teh/checkindex
| @@ -79,7 +79,7 @@ end | |||
|
|
|||
|
|
|||
| if isdefined(Base, :checkindex) | |||
| Base.checkindex(::Type{Bool}, ::UnitRange, ::NAtype) = | |||
| Base.checkindex(::Type{Bool}, ::AbstractUnitRange, ::NAtype) = | |||
There was a problem hiding this comment.
@timholy there will presumably be a window of base versions where checkindex is defined but AbstractUnitRange is not, so for the sake of bisecting base bugs I think this needs adjusting a little? I gave you collaborator access to my fork if that helps, I'm not a member of JuliaStats (are you?)
There was a problem hiding this comment.
Looks like Tim isn't a JuliaStats member. I was recently made a member so I'd be happy to help out however I can.
There was a problem hiding this comment.
I was just gonna declare you my next-most-active surrogate for Milan while he's on vacation ☀️
|
I'll make Milan's suggested renamings when I'm at a desk. |
and indexshape to _index_shape
tkelman
changed the title
|
okay if this is green I think it's good to merge, and I suppose tag too since there haven't been many serious changes since the last tag |
|
Yep, I agree it should be merged and tagged. Idk how permissions work since you aren't a JuliaStats member but if you can't tag it then I can tag it a little later Thanks so much for doing this! |
|
I could submit a tag PR to metadata, but someone with commit access would need to push the git tag here |
Which PR's? This fixes matters on 0.5, but I don't think there's anything in here as far as DataArrays API's that other packages would rely on? |
|
I assume this would fix the issue that's breaking the DataFrames and GLM tests on 0.5. Regarding the PRs, my brain was stuck in yesterday when I had a couple of outstanding PRs. (Insert hackneyed joke about Mondays here) |
|
Ah, I hope so. What you would do there is add upper bounds on the |
|
annoying thing about squashes is they don't preserve multiple-author information very well |
|
Oh you're right, I hadn't noticed that. That's too bad. |
|
Is there anything that can be done that's somewhere between a straight merge and a squash-n-merge that would better preserve the author information? Or in cases like these should regular merges be preferred? |
|
Depends what you care more about - skipping the intermediate commits (not all of which may have worked perfectly) in the future if you need to bisect through the history of the package, or preserving the authorship information. I don't think git allows you to have more than one author on a single commit, though you could manually edit the message to at least acknowledge multiple authors in the content of the log? |
|
Should I revert the commit, modify the log to mention Tim, and recommit? It'd be nice to have him in there. In the future I'll make sure all contributors are mentioned in squashed commits before merging. Sorry for the oversight here. |
|
I don't think he'll take it personally, and the more-linear history might help matters for some future bisect, you never know. You could also do the thing that Stefan has done on base a few times and make an empty merge commit that has both current master and the granular series of commits from this branch as parents. Though I don't know what that would do to bisects... |
|
I'm not sure how to do that but I'd be happy to figure it out if that's what you think would be best. |
|
I'm not positive how to do it either, I don't think it's worth worrying about. |
|
Okay. Well, to anyone reading this, Tim Holy rocks and has made invaluable contributions to this PR! 🎸 🤘 👏 |
|
Thanks guys. As @tkelman surmises, credit is much less important than avoiding possibly-dangerous git shenanigans, so no sweat. |
trying to work around OneTo issue on nightly