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

feat(Seq.traverse/sequence*)!: Yield arrays #310

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bartelink
Copy link
Contributor

@bartelink bartelink commented Feb 17, 2025

Instead of yielding a [lazy] sequence as the bodies of the Error or Ok state arising from the traverse and sequence results, yields Arrays directly. This offers two advantages:

  • better pattern matching options - e.g. can match on Ok [||], Error [||], or use _.Length on the Error or Ok bodies (vs having to to when Seq.length etc)
  • more efficient implementation (the current uses all sorts of temps via use of Seq.rev etc

Resolves #254

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

In general, the signatures can be used interchangeably, but its undeniable that this is a binary breaking change.

Checklist

  • Build and tests pass locally

@bartelink
Copy link
Contributor Author

bartelink commented Feb 17, 2025

cc @1eyewonder @TheAngryByrd My original impl was #255

I believe this correctly walks the sequences and combines successes and failures for the A variants (I reworded the docs as they suggested other semantics)

IMO this impl should be relatively efficient, but my goal in using and returing arrays is more about the usage experience, and intuitive ease of reading the impl than actual perf goals.

If you guys feel it's warranted, I'm not objecting to people tuning it a bit, though I can't say I'm personally that concerned as the Seq.rev being gone is already a huge win IMO


My main questiion/review comment would be whether you'd like me to apply this impl approach to the other variants - given they are relatively new, I guess now is the time, if it's ever to happen? I personally don't touch code that uses them so am not directly concerned (though consistency of approach is important for the lib in general of course)

@1eyewonder
Copy link
Contributor

1eyewonder commented Feb 17, 2025

@bartelink If you're looking to yield arrays directly, is there a reason why we are trying to implement in the Seq module as opposed to the existing Array module? The reason why I ended up converting this originally was due to needing lazy loading in very large sequences I was working with at the time. You can see the history shown here #277 if you didn't see this already. I'd also suggest adding benchmarks to the existing benchmark tests if we want to prove this is a more efficient implementation.

@bartelink
Copy link
Contributor Author

bartelink commented Feb 17, 2025

@1eyewonder sorry for the slow response - was porting the rest...

This is not about perf - I won't be surprised if it beats the current impl, but that's not the point - the point is a cleaner output contract. My references to perf in my comment above is more about the fact that I can well imagine there being some common cases worthy of special casing over the base impl (at the price of more code for people to review and/or traverse in perpetuity, which is why this impl keeps it simple for now)

The semantics should be 100% compatible with your cited need (lazy consumption of the input)


ASIDE: I don't see any point in having equivalents for Array, ResizeArray or anything else; this already covers what I see as the needs:

  1. lazy consumption, stopping when the outcome is decided
  2. minimal allocations and good general perf
  3. be able to pattern match and/or efficiently count the outputs as necessary

I tend to use immutable arrays over Lists, so that's less of a concern for me. Given the nature of how they work, it may well be that a specific implementation might be warranted for those if the requirement is for an optimal implementation (though in general having List.rev/Seq.rev and/or list comprehensions that are doing the moral equivalent of appending to the list is going to make that hard to achieve)

The TL;DR of all this is that the base impl in #255 is very boring and easy to read, and more complex implementations with lots of nesting, recursion and.or allocations are the ones that should have a burden of justifying that complexity

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.

feat(Seq): Add sequenceResultA
2 participants