MAINT: fix incompatible type comparison in numpy.lib.utils.info #17498
Conversation
|
No test? Under what code path is this hit? |
See ticket numpy#17490 Add test to cover the change Fix if statement to ignore builtin methods that are not printed
|
Hey @mattip thanks for pointing that out. I added a test and actually found that the if statement needed to be more specific. I modified it to ignore builtin methods, since they are not printed anyways. This makes sure that the "Methods:" header won't be printed if there are no methods to document below the header. |
There was a problem hiding this comment.
Generally looks good, a couple suggestions for the test that I would argue make it more readable but that's really just my opinion!
No test? Under what code path is this hit?
AFAICT np.info doesn't appear to be tested anywhere else, so I'm not sure the test is necessary but I think the overall change is an improvement.
See ticket numpy#17490 Clean up test style Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
numpy/lib/utils.py
Outdated
|
|
||
| if any(meth[0] != '_' for meth in methods): | ||
| print("\n\nMethods:\n", file=output) | ||
| for meth in methods: | ||
| if meth[0] == '_': |
There was a problem hiding this comment.
Perhaps better as:
public_methods = [meth for meth in methods if meth[0] != '_']
if public_methods:
...
There was a problem hiding this comment.
Thanks, that definitely improves readability. If we have the list public_methods available, might as well change this
for meth in methods:
if meth[0] == '_':
continueto this
for meth in public_methods:since the meth[0] == '_' check is redundant.
Do you agree?
There was a problem hiding this comment.
That's what I was thinking :)
There was a problem hiding this comment.
Great! I just updated it.
See ticket numpy#17490 Clean up numpy.lib.utils.info
|
Thanks for the first contribution @stevejoachim! Given how useless a function |
|
No problem, I appreciated getting the detailed feedback on it. Great way to learn. Thanks! |
See ticket #17490