ibis icon indicating copy to clipboard operation
ibis copied to clipboard

Published 20 hours ago • verified publisher icon ibis-project

refactor: deprecate register api

Open ncclementi opened this issue 1 year ago • 7 comments

Looks like I messed something up in the rebase, I'll fix it soon.

  • Closes https://github.com/ibis-project/ibis/issues/8735

ncclementi avatar Apr 02 '24 17:04 ncclementi

I modified the tests in question to use read_csv or read_parquet I wasn't sure what the name of the test should be or if we need them given that there are separate read_csv and read_parquet, though the register tests seem to be more complete. Happy to re-visit.

ncclementi avatar Apr 09 '24 20:04 ncclementi

Update regarding adapting test_register_csv and similar to use con.read_csv

Turns out that doing this would require to modify the tests quite a bit given that now this will trigger some XPASS failures, and the tests will become way more convoluted than what they are at the moment.

After chatting with @cpcloud, the suggestion was to leave the tests as they are now, and discuss a decision for when we remove the register api completely.

ncclementi avatar Apr 10 '24 20:04 ncclementi

Ok, I'm good with the changes here, but I think we need a consensus from a few folks on whether to merge now or not. If we want to add this in Ibis 9.0, I think we also need to handle standardizing read_in_memory xref #6593 before we release, because it seems irresponsible to deprecate functionality without offering a replacement.

gforsyth avatar Apr 11 '24 14:04 gforsyth

@gforsyth Brings up a good point. Let's hold off on merging to 9.0 for now to avoid delaying its release. We can merge this soon after 9.0 is cut (not immediately, just to wait for the bug reports to come in and cut a 9.0.1 or 9.1 bug fix release).

cpcloud avatar Apr 12 '24 11:04 cpcloud

I just rebased, I think when CI finishes we can get this one in since 9.0 is out

ncclementi avatar May 21 '24 15:05 ncclementi

@gforsyth should we get this one in, or are there still concerns merging this deprecation without having an alternative read_in_memory api?

ncclementi avatar May 23 '24 17:05 ncclementi

should we get this one in, or are there still concerns merging this deprecation without having an alternative read_in_memory api?

My preference is to have a replacement available -- I lost track of that work, but let me see if I can pick it back up. In the interim, we'll want to update all the deprecated as of messages to be 9.1 instead of 9.0

gforsyth avatar May 23 '24 18:05 gforsyth

Ready, waiting for https://github.com/ibis-project/ibis/pull/9251 to be merged.

ncclementi avatar May 28 '24 20:05 ncclementi