Don't print empty byte strings for no metadata

[hyanwong]

Fixes #2349 but no tests yet: I want to see if this is the right approach: i.e. should an empty metadata schema be treated as False when viewed as a bool? Thoughts @benjeffery ?

[codecov]

Codecov Report

Merging #2351 (ef4c32e) into main (7a1a1a0) will decrease coverage by 1.99%.
The diff coverage is 100.00%.

❗ Current head ef4c32e differs from pull request most recent head 42570fc. Consider uploading reports for the commit 42570fc to get more accurate results

@@            Coverage Diff             @@
##             main    #2351      +/-   ##
==========================================
- Coverage   93.29%   91.29%   -2.00%     
==========================================
  Files          28       28              
  Lines       26764    27936    +1172     
  Branches     1227     1507     +280     
==========================================
+ Hits        24969    25504     +535     
- Misses       1762     2370     +608     
- Partials       33       62      +29     

Flag	Coverage Δ
c-tests	92.25% <ø> (+0.01%)	⬆️
lwt-tests	89.05% <ø> (ø)
python-c-tests	71.47% <100.00%> (+0.26%)	⬆️
python-tests	98.86% <100.00%> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
python/tskit/tables.py	98.92% <100.00%> (ø)
python/tskit/util.py	100.00% <100.00%> (ø)
python/tskit/trees.py	76.97% <0.00%> (-21.37%)	⬇️
python/_tskitmodule.c	90.68% <0.00%> (+0.01%)	⬆️
python/tskit/metadata.py	99.06% <0.00%> (+0.04%)	⬆️
c/tskit/trees.c	95.04% <0.00%> (+0.06%)	⬆️

---

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 7a1a1a0...42570fc. Read the comment docs.

[jeromekelleher]

I'm not sure this is the best way to do things anyway - isn't the point that printing out the "b'...'" is unhelpful, and we should just strip the "b" and quotes out of the rendered metadata? I don't see any advantage to keeping them in.

So, we define a function

def render_metadata(md):
       s = str(md)
       if isinstance(md, bytes):
            # Strip leading b' and trailing '
            s = s[2:-1]
       return s

and use this for all the table output functions?

Of course, util.render_metadata should also probably call util.truncate_string_end so we just call that util.render_metadata in the table text display code.

[hyanwong]

I'm not sure this is the best way to do things anyway - isn't the point that printing out the "b'...'" is unhelpful, and we should just strip the "b" and quotes out of the rendered metadata? I don't see any advantage to keeping them in.

Happy to do this: I was taking a more conservative approach and assuming that we would want to distinguish e.g. metadata of b'{"a": 1}' from json.dumps({"a": 1}). I think removing the b' and ' would make these two indistinguishable, right?

[jeromekelleher]

🤷 We probably don't expect there to be much metadata-with-no-schema out there, so maybe having the ugly b'stuiff' is useful.

But definitely lets not get into what the boolean interpretation of a schema should be.

[benjeffery]

I'd like to keep this really simple and only replace b'' with "" for display purposes and keep everything else as-is. This fixes the most common case of ugliness without causing confusion.

[hyanwong]

I'd like to keep this really simple and only replace b'' with "" for display purposes and keep everything else as-is. This fixes the most common case of ugliness without causing confusion.

I.e.

"" if isinstance(row.metadata, bytes) and len(row.metadata) == 0 else util.truncate_string_end(str(row.metadata))

?

[benjeffery]

I.e.

"" if isinstance(row.metadata, bytes) and len(row.metadata) == 0 else util.truncate_string_end(str(row.metadata))

?

"" if row.metadata == b'' else util.truncate_string_end(str(row.metadata))

[hyanwong]

Can we get away with a simple == if row.metadata is any arbitrary thing? Are we guaranteed that it's never None, or a bool, or an np array, etc? I guess so?

[jeromekelleher]

How about we use a function, like this? Then we avoid distributing this logic around many different places.

[benjeffery]

How about we use a function, like this? Then we avoid distributing this logic around many different places.

Happy with the function, but I think it should only check for b'' as it needs to be clear when metadata is bytes.

[hyanwong]

Done. I stuck a line in test_tables.py: test_str to put a blank metadata value in there, but perhaps that's excessive as the function is tested in test_utils.py.

[hyanwong]

Current test failure is a codecov fault, I think, nothing to do with this PR.

[benjeffery]

I added a changelog, as technically this is a breaking change.