Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Md null schema #2720

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Md null schema #2720

wants to merge 3 commits into from

Conversation

hyanwong
Copy link
Member

@hyanwong hyanwong commented Mar 2, 2023

While thinking with metadata for the tskit poster, I remembered this on the queue. As discussed with @benjeffery - this is mostly a documentation PR, but it also changes behaviour so that str(tskit.MetadataSchema(schema=None)) returns "Null_schema" rather than None, which was pretty confusing, because tskit.MetadataSchema(schema=None) is None always returns False.

@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Merging #2720 (c3af8b0) into main (c12f384) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2720      +/-   ##
==========================================
- Coverage   94.08%   94.07%   -0.01%     
==========================================
  Files          29       29              
  Lines       28516    28518       +2     
  Branches     1609     1610       +1     
==========================================
+ Hits        26828    26829       +1     
  Misses       1653     1653              
- Partials       35       36       +1     
Flag Coverage Δ
c-tests 92.36% <ø> (ø)
lwt-tests 89.05% <ø> (ø)
python-c-tests 70.89% <100.00%> (+<0.01%) ⬆️
python-tests 99.05% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
python/tskit/metadata.py 99.03% <100.00%> (+<0.01%) ⬆️
python/tskit/cli.py 96.18% <0.00%> (-0.77%) ⬇️

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 c12f384...c3af8b0. Read the comment docs.

Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@hyanwong
Copy link
Member Author

Having looked at #2126, I wonder if, instead, we should report the str() as MetadataSchema(None), and indeed report all schemas like that? This would fully address #2126

@hyanwong hyanwong marked this pull request as draft July 22, 2023 12:17
@hyanwong
Copy link
Member Author

I now think we should use this for the str representation of a schema:

    def __str__(self) -> str:
        if isinstance(self._schema, collections.OrderedDict):
            s = pprint.pformat(dict(self._schema))
        else:
            s = pprint.pformat(self._schema)
        if "\n" in s:
            return f"MetadataSchema(\n{s}\n)"
        else:
            return f"MetadataSchema({s})"

What do you think, @benjeffery ? Will this break anything?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants