Skip to content
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

Fix ExploreRecursive stopAt condition and add tests #229

Merged
merged 5 commits into from
Aug 19, 2021

Conversation

adlrocha
Copy link
Contributor

This PR:

  • Fixes the bug in ExploreRecursive stopAt condition.
  • Adds a bunch of test cases to test the expected functionality of this feature.

@warpfork
Copy link
Collaborator

Thank you so much for creating test cases for this! Tests are life.

Can I also beg you to make a PR to our specs and briefly describe how stopAt is working now? Adding another paragraph to the end of the ExploreRecursive docs, here, should be about the right spot.

In particular I think what we've settled on is that the stopAt condition can cause the selector to stop anywhere within recursion sequence, not just at its edges. That's in need of some prose, because I'm not sure whether that would be what people will reliably intuit if it's not discussed.

@adlrocha
Copy link
Contributor Author

@warpfork, it was straightforward as you promised :) I love when "oh come on is trivial" is actually trivial. Let me know if there is anything else missing. Thanks!

@warpfork
Copy link
Collaborator

I'm thrilled it worked out 👯

This looks good to me! If you can rebase onto master, so I can click 'merge' and also keep your individual commits, that's ideal. (Or we can squash if you don't care for the history; no big deal.)

If you really want to go full hulk on this... you can now also add test fixtures as serial data in the langauge-agnostic docs+specs repo, and get them tested in here, as of #231 and ipld/ipld#130 . I can work through that flow with you if you want, but also I won't hold you to it -- you've done a lot here already.

@adlrocha
Copy link
Contributor Author

Merged

If you really want to go full hulk on this... you can now also add test fixtures as serial data in the langauge-agnostic docs+specs repo, and get them tested in here, as of #231 and ipld/ipld#130 .

Cool, I'll look at it tomorrow and ask you for a walkthrough if needed. Thanks!

@warpfork warpfork merged commit d7e93a8 into ipld:master Aug 19, 2021
@aschmahmann aschmahmann mentioned this pull request Sep 30, 2021
62 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants