-
Notifications
You must be signed in to change notification settings - Fork 12
feat: improve zipWidth element count #210
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -534,25 +534,80 @@ export interface IMaybe<T> extends IMonad<T> { | |
| flatMapMany<R>(fn: (val: NonNullable<T>) => Promise<R>[]): Promise<IMaybe<NonNullable<R>[]>> | ||
|
|
||
| /** | ||
| * Combines this Maybe with another Maybe using a combiner function. | ||
| * | ||
| * If both Maybes are Some, applies the function to their values and returns | ||
| * a new Some containing the result. If either is None, returns None. | ||
| * | ||
| * Combines this Maybe with one or more other Maybes using a combiner function. | ||
| * | ||
| * If all Maybes are Some, applies the function to their values and returns | ||
| * a new Some containing the result. If any is None, returns None. | ||
| * | ||
| * @typeParam U - The type of the value in the other Maybe | ||
| * @typeParam R - The type of the combined result | ||
| * @param other - Another Maybe to combine with this one | ||
| * @param fn - A function that combines the values from both Maybes | ||
| * @returns A new Maybe containing the combined result if both inputs are Some, otherwise None | ||
| * | ||
| * @returns A new Maybe containing the combined result if all inputs are Some, otherwise None | ||
| * | ||
| * @example | ||
| * // Combine user name and email into a display string | ||
| * // Combine two values | ||
| * const name = maybe(user.name); | ||
| * const email = maybe(user.email); | ||
| * | ||
| * | ||
| * const display = name.zipWith(email, (name, email) => `${name} <${email}>`); | ||
| * // Some("John Doe <john@example.com>") if both name and email exist | ||
| * // Some("John Doe <john@example.com>") if both exist | ||
| * // None if either is missing | ||
| * | ||
| * @example | ||
| * // Combine three values | ||
| * const firstName = maybe(user.firstName); | ||
| * const lastName = maybe(user.lastName); | ||
| * const email = maybe(user.email); | ||
| * | ||
| * const contact = firstName.zipWith(lastName, email, (first, last, email) => ({ | ||
| * fullName: `${first} ${last}`, | ||
| * })); | ||
| * // Some({ fullName: "John Doe", email: "john@example.com" }) if all exist | ||
| * // None if any is missing | ||
| * | ||
| * @example | ||
| * // Combine many values | ||
| * const result = a.zipWith(b, c, d, e, (a, b, c, d, e) => a + b + c + d + e); | ||
| */ | ||
| zipWith<U extends NonNullable<unknown>, R>(other: IMaybe<U>, fn: (a: NonNullable<T>, b: U) => NonNullable<R>): IMaybe<R> | ||
| zipWith<U extends NonNullable<unknown>, R>( | ||
| other: IMaybe<U>, | ||
| fn: (a: NonNullable<T>, b: U) => NonNullable<R> | ||
| ): IMaybe<R> | ||
|
|
||
| zipWith<U extends NonNullable<unknown>, V extends NonNullable<unknown>, R>( | ||
| m1: IMaybe<U>, | ||
| m2: IMaybe<V>, | ||
| fn: (a: NonNullable<T>, b: U, c: V) => NonNullable<R> | ||
| ): IMaybe<R> | ||
|
|
||
| zipWith<U extends NonNullable<unknown>, V extends NonNullable<unknown>, W extends NonNullable<unknown>, R>( | ||
| m1: IMaybe<U>, | ||
| m2: IMaybe<V>, | ||
| m3: IMaybe<W>, | ||
| fn: (a: NonNullable<T>, b: U, c: V, d: W) => NonNullable<R> | ||
| ): IMaybe<R> | ||
|
|
||
| zipWith<U extends NonNullable<unknown>, V extends NonNullable<unknown>, W extends NonNullable<unknown>, X extends NonNullable<unknown>, R>( | ||
| m1: IMaybe<U>, | ||
| m2: IMaybe<V>, | ||
| m3: IMaybe<W>, | ||
| m4: IMaybe<X>, | ||
| fn: (a: NonNullable<T>, b: U, c: V, d: W, e: X) => NonNullable<R> | ||
| ): IMaybe<R> | ||
|
|
||
| zipWith<U extends NonNullable<unknown>, V extends NonNullable<unknown>, W extends NonNullable<unknown>, X extends NonNullable<unknown>, Z extends NonNullable<unknown>, R>( | ||
| m1: IMaybe<U>, | ||
| m2: IMaybe<V>, | ||
| m3: IMaybe<W>, | ||
| m4: IMaybe<X>, | ||
| m5: IMaybe<Z>, | ||
| fn: (a: NonNullable<T>, b: U, c: V, d: W, e: X, f: Z) => NonNullable<R> | ||
| ): IMaybe<R> | ||
|
|
||
| // Variadic overload for 5+ Maybes | ||
| zipWith<R>( | ||
| ...args: [...IMaybe<NonNullable<unknown>>[], (...values: NonNullable<unknown>[]) => NonNullable<R>] | ||
| ): IMaybe<R> | ||
|
Comment on lines
+609
to
+612
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new overloads cover arities 2–6 plus a variadic signature, which is powerful but also introduces a somewhat tricky call contract: callers must always pass Given how overloaded this API has become, it might be worth tightening the public surface a bit:
Right now consumers could still do something like SuggestionYou could tighten the contract and improve debuggability either by documenting the calling convention explicitly or by validating the last argument at runtime. For example, a lightweight runtime guard: zipWith<R>(
...args: [...IMaybe<NonNullable<unknown>>[], (...values: NonNullable<unknown>[]) => NonNullable<R>]
): IMaybe<R>;and in the implementation: public zipWith<R>(...args: unknown[]): IMaybe<R> {
if (this.isNone()) {
return new Maybe<R>();
}
const fn = args[args.length - 1];
if (typeof fn !== 'function') {
throw new TypeError('zipWith: last argument must be a function');
}
const maybes = args.slice(0, -1) as IMaybe<unknown>[];
// ...rest stays the same
}This keeps the strong typing while turning obvious misuses into clear runtime errors instead of silent mis-computation. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -301,7 +301,23 @@ export class Maybe<T> implements IMaybe<T> { | |
| .catch(() => new Maybe<NonNullable<R>[]>()) | ||
| } | ||
|
|
||
| public zipWith<U extends NonNullable<unknown>, R>(other: IMaybe<U>, fn: (a: NonNullable<T>, b: U) => NonNullable<R>): IMaybe<R> { | ||
| return this.flatMap(a => other.map(b => fn(a, b))) | ||
| public zipWith<R>(...args: unknown[]): IMaybe<R> { | ||
| if (this.isNone()) { | ||
| return new Maybe<R>() | ||
| } | ||
|
|
||
| const fn = args[args.length - 1] as (...values: unknown[]) => NonNullable<R> | ||
| const maybes = args.slice(0, -1) as IMaybe<unknown>[] | ||
|
|
||
| const values: unknown[] = [this.value] | ||
|
|
||
| for (const m of maybes) { | ||
| if (m.isNone()) { | ||
| return new Maybe<R>() | ||
| } | ||
| values.push(m.valueOrThrow()) | ||
| } | ||
|
|
||
| return new Maybe<R>(fn(...values)) | ||
|
Comment on lines
+304
to
+321
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The implementation assumes that the last argument is always a function and that there is at least one At runtime, if
Given the library appears to aim for robustness in other places (e.g. SuggestionConsider adding minimal runtime validation for the function and argument count, for example: public zipWith<R>(...args: unknown[]): IMaybe<R> {
if (this.isNone()) {
return new Maybe<R>()
}
if (args.length === 0) {
throw new Error('zipWith requires at least one other Maybe and a combiner function')
}
const fn = args[args.length - 1]
if (typeof fn !== 'function') {
throw new Error('zipWith last argument must be a function')
}
const maybes = args.slice(0, -1) as IMaybe<unknown>[]
const values: unknown[] = [this.value]
for (const m of maybes) {
if (m.isNone()) {
return new Maybe<R>()
}
values.push(m.valueOrThrow())
}
return new Maybe<R>((fn as (...values: unknown[]) => Nonnullable<R>)(...values))
}This keeps the overload behavior intact while offering a clearer failure mode when misused from less strict code. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion. |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current variadic overload type:
implicitly allows calling
zipWithwithout any other Maybes, e.g.maybe(1).zipWith(x => x + 1)at the type level (the tuple[]is a validIMaybe<NonNullable<unknown>>[]). This contradicts the JSDoc examples and the intended semantics of “Combines this Maybe with one or more other Maybes”.This mismatch between the documented contract and the type-level contract can lead to confusing or unsafe usage, especially given the generic
unknown-based typing here. It would be better to encode “at least one other Maybe” in the signature itself.Suggestion
You can enforce the presence of at least one additional
Maybein the variadic overload by changing the tuple rest pattern, for example:This preserves the existing overloads for up to five extra Maybes while making the fallback overload consistent with the documented requirement of “one or more other Maybes.” Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.