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

Merged
merged 1 commit into from
Sep 23, 2024
Merged

Md null schema #2720

merged 1 commit into from
Sep 23, 2024

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

All modified and coverable lines are covered by tests ✅

Project coverage is 86.96%. Comparing base (4430763) to head (cc49b84).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2720      +/-   ##
==========================================
- Coverage   89.72%   86.96%   -2.77%     
==========================================
  Files          29       11      -18     
  Lines       31573    24358    -7215     
  Branches     6115     4501    -1614     
==========================================
- Hits        28328    21182    -7146     
+ Misses       1853     1817      -36     
+ Partials     1392     1359      -33     
Flag Coverage Δ
c-tests 86.55% <ø> (ø)
lwt-tests 80.78% <ø> (ø)
python-c-tests 88.98% <ø> (ø)
python-tests ?

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

see 18 files with indirect coverage changes

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?

@benjeffery benjeffery force-pushed the md-null-schema branch 4 times, most recently from b01a6b7 to 8406c09 Compare September 20, 2024 16:15
@benjeffery benjeffery marked this pull request as ready for review September 20, 2024 16:17
@benjeffery
Copy link
Member

I've rebased this and make the string tskit.MetadataSchema(None)

@benjeffery benjeffery added AUTOMERGE-REQUESTED Ask Mergify to merge this PR and removed AUTOMERGE-REQUESTED Ask Mergify to merge this PR labels Sep 23, 2024
@benjeffery
Copy link
Member

@Mergifyio rebase

Copy link
Contributor

mergify bot commented Sep 23, 2024

rebase

✅ Branch has been successfully rebased

@benjeffery benjeffery merged commit fb87f87 into tskit-dev:main Sep 23, 2024
12 of 13 checks passed
@mergify mergify bot removed the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Sep 23, 2024
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