Successors length limits #224
Conversation
Tagl
force-pushed
the
successors_length_limits
branch
from
0b09419
to
aaaabfb
Compare
|
Looks good from a glance! When I make a pass through the active PR list a little later I can add some tests before merging. |
|
@eliotwrobson @Tagl Looks like a great addition to me! Thank you for this! I am converting the PR to Draft state until more tests are added. |
|
@Tagl added a couple of test cases and encountered a bug I commented on: f95b0d1#diff-404c46da3c65463b61109992f94cfd102f9f20f5467f1916bf9bf50ff89c7176R1894-R1895 I think this can be avoided just by popping from the char stack if the size goes over the length limit. |
|
@Tagl for this, should the stop condition be changed to when the algorithm hits the string of all the lexicographically last character of length Edit: just a heads up that we'll probably be closing up the PyOpenSci review soon, and it would be awesome if the release after that could include the changes here. |
|
I'll take a look tomorrow |
|
Should be fixed now, I added one more edge case to test for as well. |
There was a problem hiding this comment.
Looks good now that we have that edge case sorted out, thanks! Going to go ahead and approve. @Tagl do you think this is good to merge now?
|
Yes, I believe so. |
This reverts commit 39ca92a.
* Succesor length limits * Added extra test cases and comment about bug * Fix infinite loop bug when length is limited. * Re-enable disabled test * Add an edge case test for predecessor * Formatting --------- Co-authored-by: Eliot Robson <eliot.robson24@gmail.com>
Closes #207
Might be wise to add a few more tests.