re-write columnize, with intermediate step. #1875
Merged
+117
−58
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Is there no way to test deterministically? A random test that may pass even with incorrect code doesn't sound ideal. |
|
There are already some derminalistic test, the random one is ment to hopefully catch some edge case we missed (if there is some), then we could fix, and add this specific case as a deterministic test... otherwise we should test for all combinaison of length... |
|
OK, that makes sense - there's a deterministic test for this problem, and a random test to flag up potential unknown problems. Thanks, |
| def _find_optimal(rlist , sepsize=2 , displaywidth=80): | ||
| """Calculate optimal info to columnize a list of string""" | ||
| for nrow in range(1, len(rlist)+1) : | ||
| chk = [max(l) for l in _chunks(rlist, nrow) ] |
There was a problem hiding this comment.
IMO, when there's no extra logic, map is still clearer than a listcomp (it's also faster):
chk = map(max, _chunks(rlist, nrow))
|
A few small points, but overall looks great, thanks! |
fix test that where wrong, add some others. fix ipython#1860
|
ok, thanks, fixed, also in #1851 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix test that where wrong, add some others.
fix #1860
Maybe not the fastest implementation ever, but allow some granularity, and intermediate state i'll need for #1851. More over, it does more in less lines (docstrings and test excluded :-) ).
Nice thing with this granularity is that we could add stretching separators in one line if necessary.
Right now, it finds the right number of columns by bruteforcing,
we could try to bisect if we start hitting high number of items, but i'm not sure it is necessary.
Output of the new and old function were identical on around 20000 random test case, except when the old columnize was wrong (wider output than displaywidth).
I added a random test to hopefully catch edges cases (fail with a few percent probability on old columnize, haven't failed with new in thousand of sample) .
As it is a random test, I wasn't sure on how to keep information about it, so I added print statements and
assert Falseis there a better way ?