ibis
ibis copied to clipboard
ibis-project
feat(api): move from .case() to .cases()
Redo of #7914 (with substantive changes) and #9039 (merely switching the base repo to the correct one, my fork)
Instead of removing the old .case() APIs, I deprecate them here. There are a few tests for them to try to ensure we didn't break anyone. Open to thinking about this deprecation path differently. See this comment thread below.
EDIT: duh, it's because they don't guarantee row order. Updated the assertions to be order-independent.
Any idea as to why the datafusion, exasol, and risingwave column tests are failing? I still have trouble getting those backends running on my M1 so I can't debug locally very well.
@NickCrews would you mind fixing the conflicts and CI, then I'll nudge the team to get you a review soon.
Thanks for the poke. I have a lot of other PRs in flight at the moment with ibis, many of which I think are a lot closer to the finish line than this, I'd love to get those merged first before I add another ball to juggle.
One thing that should get merged first is #9334, where I fix a datashape bug that is exposed in this PR. Once that is in, then this PR will be simpler.
Once https://github.com/ibis-project/ibis/pull/9559 lands then I think the scope of this will be significantly reduced and this should be much easier, thank you for your reviews!
OK, this is much simpler now, there are no functionality changes, only API changes.
The one question in the air is what to do with the old APIS. Can you respond in the thread and then I can adjust things?
Going through our PR backlog - @cpcloud, can you please give this another review when you have a moment?
Thinking about this PR some more, I'm not sure why we're going to all this trouble.
Do we really need to remove the existing case API? What problems is it causing or what additional maintenance does it add?
Right now cases is implemented in terms of the more primitive case, which is much less complex than what is being proposed here.
I think I am now -1 on this PR altogether. It seems like it might be fine to break cases and make it variadic for 10, but removing case seems wholly unecessary.
I want the user experience to be consistent, and only one of the APIs to be surfaced in the docs and all the examples. I think I'd be fine not deleting the old API, and not even deprecating it, but just removing it from the docs. Also maybe removing it from tab complete? By putting it under gettattr or something?? In terms of maintenance cost I agree its not bad.
I do want to think about this not in terms of the cost to do the transition between these two states, which is a one time thing, and more in terms of how does the quality of the final state compare to how it was before for the user?
Also, if we're worried about breaking people, I believe it is a thing where we can ship a script that goes through their source code and rewrites the change for them (for the easy instances at least). There's a project that does this I think, just blanking on the name
I'm still motivated to get this merged. I am trying to keep the original post at the top of this PR in sync with the changes, to provide a good summary of the changes, why we are doing them, and alternatives considered. I think I have mitigated some of your concerns above. Please read that and see what you think? Thanks!
@cpcloud looks like the docs CI is failing just for permissions issues, so I think this is good to merge! (If you confirm then in the future I will be confident to just merge if I see only this CI failure)
Please do not merge anything that has CI failures.
This job in particular is testing whether the documentation can be built, and that definitely is a hard requirement for merging a PR.
@cpcloud I added the breaking change label to this PR, since it does hard-break Value.cases(), as described in the OP at the top.
Ah oops. Well, we'll have to add it manually after the release as it's already merged.