Add update row to low level python

[benjeffery]

Initial low level work for #1545
WIP - only individuals done

[benjeffery]

@jeromekelleher Can I get a quick review here before the copy-paste-athon?

[codecov]

Codecov Report

Merging #1597 (4d47e3b) into main (8950b64) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 4d47e3b differs from pull request most recent head 4c2c67c. Consider uploading reports for the commit 4c2c67c to get more accurate results

@@            Coverage Diff             @@
##             main    #1597      +/-   ##
==========================================
+ Coverage   93.62%   93.65%   +0.03%     
==========================================
  Files          27       27              
  Lines       23075    23237     +162     
  Branches     1079     1079              
==========================================
+ Hits        21603    21763     +160     
- Misses       1438     1440       +2     
  Partials       34       34              

Flag	Coverage Δ
c-tests	91.85% <ø> (ø)
lwt-tests	92.97% <ø> (-0.44%)	⬇️
python-c-tests	95.37% <100.00%> (+0.04%)	⬆️
python-tests	98.80% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
python/_tskitmodule.c	92.15% <100.00%> (+0.30%)	⬆️
python/lwt_interface/tskit_lwt_interface.h	95.05% <0.00%> (-0.37%)	⬇️
python/tskit/trees.py	97.95% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8950b64...4c2c67c. Read the comment docs.

[jeromekelleher]

Hmm, not sure about this pattern. I see what you're trying to do though, and avoiding duplicating all the arg parsing logic is definition worth doing.

Let me have a think about it and see if I can come up with anything better. So, hold off on the copy-paste-athon for a bit if you don't mind, please.

[jeromekelleher]

I've had a look at this, and actually the IndividualTable is the wrong place to start as it's by far and away the most complicated example. For the other tables, it'll be much simpler to just copy and paste and tweak in the obvious way than to try and factor out the common code. Try seeing what it looks like for EdgeTable, and then consider which option (i.e., trying to factor out the argument parsing code, or just copy-paste-altering) would be easier for a new contributor to understand.

[jeromekelleher]

LGTM, minor points on type safety

[jeromekelleher]

Looks good, thanks @benjeffery. Unfortunately I think we do need to get a bit more test coverage on the argument parsing error and default handling - perhaps we could add a handful of update_row invocations to where we're doing this stuff for add_row? Exhastive testing of all the options would be really tedious and a bit pointless, but we want some coverage on the error cases for the new methods (I see we do have good tests for the row_id case, all right though, which does give a lot of assurance that the error handling code is working properly).

I'm paranoid about this error handling stuff as it has turned into a real time-sink in the past trying to track down segfaults caused by weird behaviour in small mistakes with ArgParseTuple

[benjeffery]

perhaps we could add a handful of update_row invocations to where we're doing this stuff for add_row?

In that case I'm tempted to update the old add_row code so that the tests can be identical.

[jeromekelleher]

Changes look good, great to cut down on our technical dept a bit like this.

[benjeffery]

Ok, I think this is good to go. I didn't add the update tests where we are doing add row tests as they are in the higher-level tests. I also spotted some missing add_row tests.

[jeromekelleher]

Looks great, thanks @benjeffery!