-
Notifications
You must be signed in to change notification settings - Fork 36
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
Added pretty representation of ProjectSchema for IPython #324
Conversation
Codecov Report
@@ Coverage Diff @@
## master #324 +/- ##
==========================================
+ Coverage 76.19% 76.21% +0.01%
==========================================
Files 43 43
Lines 7078 7083 +5
==========================================
+ Hits 5393 5398 +5
Misses 1685 1685
Continue to review full report at Codecov.
|
signac/contrib/schema.py
Outdated
@@ -136,6 +138,15 @@ def __str__(self): | |||
def __repr__(self): | |||
return "{}(<len={}>)".format(type(self).__name__, len(self)) | |||
|
|||
def _repr_html_(self): | |||
html = "{<br>" |
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.
I think we should still include the type name for the project before the opening bracket. pandas
does not, but in this case given it will look like a dictionary otherwise, so I think we should.
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.
Looks good, I have one comment to address.
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.
@vishav1771 I made a suggested code snippet, and I applied it (it's somewhat simpler and uses existing formatting logic that we already trust). For testing, I added a few lines to the class TestProjectRepresentation
in test_project.py
. I think that will make this PR ready to merge with one other approval (@b-butler?). This is a small convenience feature so I'm fine with fast-tracking it and merging quickly.
signac/contrib/schema.py
Outdated
html = "{<br>" | ||
for key in sorted(self._schema): | ||
values = self._schema[key] | ||
if values: | ||
html += "<pre> '{}': '{}',</pre>".format(key, self._fmt_values(values, 5)) | ||
html += "}" | ||
return html |
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.
I think Brandon has a good recommendation above. @vishav1771 It looks like this PR is re-creating the same formatting logic that is in __str__
. I suggest the following code snippet -- I'm happy with this output (screenshot below).
html = "{<br>" | |
for key in sorted(self._schema): | |
values = self._schema[key] | |
if values: | |
html += "<pre> '{}': '{}',</pre>".format(key, self._fmt_values(values, 5)) | |
html += "}" | |
return html | |
import html | |
html_repr = "<strong>" + html.escape(repr(self)) + "</strong>" | |
html_repr += "<pre>" + str(self) + "</pre>" | |
return html_repr |
@vishav1771 One more thing - you might want to update your fork's |
I also reverted the changes to |
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
Description
Added
_repr_html_
method inProjectSchema
Motivation and Context
Resolves #314
Types of Changes
1The change breaks (or has the potential to break) existing functionality.
Checklist:
If necessary:
Example for a changelog entry:
Fix issue with launching rockets to the moon (#101, #212).