-
Notifications
You must be signed in to change notification settings - Fork 78
Add a class for svg strings #2377
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2377 +/- ##
=======================================
Coverage 93.25% 93.25%
=======================================
Files 28 28
Lines 26864 26868 +4
Branches 1229 1230 +1
=======================================
+ Hits 25053 25057 +4
Misses 1778 1778
Partials 33 33
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
59ddc22 to
0c2b72a
Compare
|
The functionality is not actually documented in the |
benjeffery
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one PEP8 nit.
python/tskit/drawing.py
Outdated
| return self.plot_min - (y - self.min_time) * y_scale | ||
|
|
||
|
|
||
| class _svg_string(str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use SVGString? As a class name it has to be capitalised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, very good point. If we think of it a an "internal use" class do we want a prefix underscore, _SVGString or are we happy that it's sort of public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the underscore as it is returned by the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, and a bit of documentation added.
|
Docs failing because |
|
As this is being returned by the public API it should probably be doc'd under https://tskit.dev/tskit/docs/stable/python-api.html#miscellaneous-classes |
Oh, very good call. Done. |
benjeffery
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
As discussed in https://github.com/tskit-dev/tskit/discussions/2376