Add table.replace_with #2389
Add table.replace_with #2389
Conversation
hyanwong
force-pushed
the
table_replace_with
branch
2 times, most recently
from
3efb515 to
fb92507
Compare
Codecov Report
@@ Coverage Diff @@
## main #2389 +/- ##
=======================================
Coverage 93.32% 93.33%
=======================================
Files 28 28
Lines 26915 26952 +37
Branches 1235 1236 +1
=======================================
+ Hits 25118 25155 +37
Misses 1763 1763
Partials 34 34
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
benjeffery
left a comment
benjeffery
left a comment
There was a problem hiding this comment.
Nice, couple of comments.
I still keep coming back to the idea of assignment as using this, unless you del table you have two copies hanging around. Will have a think, but this is good for now.
hyanwong
force-pushed
the
table_replace_with
branch
from
fb92507 to
fd3adaf
Compare
Good comments, thanks Ben.
Sure. If you can make it more efficient in a future iteration, that would be great! But as you say, merge for now and pick up later. |
benjeffery
left a comment
benjeffery
left a comment
There was a problem hiding this comment.
Sorry for delaying this @hyanwong, I wanted some time to think about it as it isn't really replace_with but overwrite_with_a_copy_of.
I think that eventually, we'll want to work out the (messy) details of actually replacing table in a collection, so this is kind of a stop-gap solution. As such I've asked in a comment to leave this undocumented for now. Please add an issue to suggest documenting it so we can discuss it.
hyanwong
force-pushed
the
table_replace_with
branch
from
fd3adaf to
76fea5c
Compare
|
Changes made, but if you have a better idea for how to do the replacement properly, then I'm happy to leave this for the time being anyway. I can always code up the functionality every time it is needed instead. |
benjeffery
left a comment
benjeffery
left a comment
There was a problem hiding this comment.
LGTM! Thanks @hyanwong!
There's a good chance that assignment is a bigger mouthful than we want to chew for the foreseeable so happy to have this in.
And output sensible error messages when trying to set tables directly.
AdminBot-tskit
force-pushed
the
table_replace_with
branch
from
76fea5c to
74e055a
Compare
| ) | ||
| try: | ||
| # Not all tables have a metadata_schema: if they do, encode it with repr | ||
| params["metadata_schema"] = repr(other.metadata_schema) |
There was a problem hiding this comment.
Why use repr here and not a direct API? Seem indirect and brittle (we might change repr to something slightly different, perhaps)
There was a problem hiding this comment.
repr is the standard way to get the bytes representation of a schema - it's used by (for example) metdata.py:812 and is well tested.
There was a problem hiding this comment.
That's fine internally maybe, but it's not a great convention. A well named method would be much clearer to the reader
3 participants
Description
And output sensible error messages when trying to set tables directly.
Fixes #1489
PR Checklist: