-
Notifications
You must be signed in to change notification settings - Fork 105
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
ENH: add utility functions for repr printing #1380
Conversation
Failure is due to Numpy 1.14.0 being installed, needs fix |
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.
Overall looking good. But I feel that the code is somewhat abusing doctests to do full scale testing. I'd prefer doctests to stay small and perhaps move something to a real external test.
|
||
|
||
def dedent(string, indent_str=' ', max_levels=None): | ||
"""Revert the effect of indentation. |
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.
IndentationOperator.inverse
!
(no, I'm not serious).
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.
😱
odl/util/utility.py
Outdated
# `max_levels` if given | ||
def num_indents(line): | ||
nindents = 0 | ||
while True: |
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.
This is a scary risk for an infinite loop. Perhaps some safety?
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 can do something about it
'real_dtype', 'complex_dtype', 'is_string', 'nd_iterator', 'conj_exponent', | ||
'writable_array', 'run_from_ipython', 'NumpyRandomSeed', | ||
'cache_arguments', 'unique', | ||
'REPR_PRECISION') |
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.
This file is getting out of hand. Perhaps we should split it (but leave that for another PR).
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.
Agreed
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.
See #1386
Next line. | ||
And another one. | ||
|
||
Multiple levels of indentation: |
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.
IMO this is too many tests, hard to read. At least add some spacing
odl/util/utility.py
Outdated
cur_line_len = indent_len | ||
|
||
for i, s in enumerate(strings[1:-1]): | ||
|
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.
Remove blankline
odl/util/utility.py
Outdated
The returned string is formatted such that it does not extend | ||
beyond the line boundary if avoidable. The line width is taken from | ||
NumPy's printing options that can be retrieved with | ||
``np.get_printoptions()``. They can be temporarily overridden |
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.
numpy.get_printoptions creates a clickable link
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.
ok
odl/util/utility.py
Outdated
beyond the line boundary if avoidable. The line width is taken from | ||
NumPy's printing options that can be retrieved with | ||
``np.get_printoptions()``. They can be temporarily overridden | ||
using the `npy_printoptions` context manager. See Examples for details. |
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.
This does not create a link
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.
It should, since it's one of our functions in the same module
Well, here the intention is to document the behavior of the function as precisely as possible, and for that, unit tests are not a good vehicle. Doctests are much better suited for that. The functions here are not really user-facing, so I took the approach of maximizing documentation for developers. |
Comments addressed, rebased. Good to go? |
Bump. Good to go in? |
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.
Gogo
This is a break-out from the monster PR #1276. It only adds the tools for better
repr
printing, thereby slightly changing how stuff works behind the scenes.Follow-up PRs will apply this to the classes in ODL and fix some doc issues.
I've also started to implement #1311 here (more specific TODO comments).