Skip to content

feature/choose-value-option#385

Open
njlr wants to merge 4 commits intofsprojects:mainfrom
njlr:feature/choose-value-option
Open

feature/choose-value-option#385
njlr wants to merge 4 commits intofsprojects:mainfrom
njlr:feature/choose-value-option

Conversation

@njlr
Copy link
Copy Markdown
Contributor

@njlr njlr commented Apr 12, 2026

  • Adds ValueOption variants for choose: TaskSeq.chooseV, TaskSeq.chooseVAsync
  • These variants can avoid allocations for better performance in some scenarios
  • The tests are a modified copy of those for choose
  • I was unsure about naming convention; happy to change it with some guidance
  • I will make a follow up PR for TaskSeq.tryPick, TaskSeq.pick etc. if desired

@github-actions
Copy link
Copy Markdown
Contributor

🤖 This is an automated response from Repo Assist.

Thanks for the contribution, @njlr! Great to see another PR from you. 🎉

A few observations on the implementation:

Implementation quality ✅
The chooseV implementation is clean and correct. It mirrors the existing choose implementation exactly — using the for item in source do pattern — which is ideal.

Naming ✅
chooseV / chooseVAsync is a reasonable naming choice for the ValueOption variant. The V suffix is a common convention in the F# ecosystem (e.g., Array.tryFindIndexV in some libraries, ValueSome/ValueNone in FSharp.Core). Within this library there's no prior V-suffix API yet, but given the direct parallel to choose/chooseAsync, the naming is clear and discoverable.

Consistency note — the PR touches four files appropriately: TaskSeqInternal.fs (implementation), TaskSeq.fs (public type), TaskSeq.fsi (signature), and the test project. The tests cover empty sequences, immutable variants, side effects, and type-changing — well done.

One small suggestion: would it be worth adding a release-notes.txt entry? Per the project conventions (see AGENTS.md), every user-visible change should have a bullet under the current version heading (1.0.0). Something like:

- adds TaskSeq.chooseV and TaskSeq.chooseVAsync for ValueOption-returning choosers

Otherwise this looks good to me! The follow-up for tryPickV/pickV sounds like a natural next step.

Generated by 🌈 Repo Assist, see workflow run. Learn more.

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@97143ac59cb3a13ef2a77581f929f06719c7402a

@dsyme
Copy link
Copy Markdown
Contributor

dsyme commented Apr 13, 2026

@njlr Looks great! Please update the release notes and we can mergem thanks

@njlr
Copy link
Copy Markdown
Contributor Author

njlr commented Apr 13, 2026

@njlr Looks great! Please update the release notes and we can mergem thanks

Done. Thanks 👍

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.

2 participants