search

emmanueltouzery / prelude-ts

Functional programming, immutable collections and FP constructs for typescript and javascript
ISC License
377 stars 21 forks source link

Option.getOrThrow() should take Error as argument #15

Closed bwbuchanan closed 6 years ago

bwbuchanan commented 6 years ago

Option.getOrThrow() currently takes a string as its argument. I think that it should take an Error instance, because throwing bare strings is generally considered poor Javascript coding practice.

Current method:

getOrThrow(message?: string): T & WithEquality {
    throw message || "getOrThrow called on none!";
}

Suggested:

getOrThrow(error?: Error): T & WithEquality {
    throw error || new Error("getOrThrow called on none!");
}
emmanueltouzery commented 6 years ago

Thanks for the suggestion! The reason it's currently implemented the way it is is that I'm concerned with someone porting a legacy codebase to prelude. If that codebase had a practice of throwing things which are not Error, then porting could become harder. On the other hand I suppose one could cast to any to make it work for that legacy's case, like

getOrThrow(<any>"oops")

not sure what's right in this case. I'll think it through a little more.

emmanueltouzery commented 6 years ago

Oh but getOrThrow takes and throws strings! Sorry I checked too fast. You're right this is wrong it should throw errors by default. Maybe it should take Error|string and if given a string, wrap the string in Error for convenience.

bwbuchanan commented 6 years ago

Sounds good. I agree.

On Thu, Aug 30, 2018 at 23:07 emmanueltouzery notifications@github.com wrote:

Oh but getOrThrow takes and throws strings! Sorry I checked too fast. You're right this is wrong it should throw errors by default. Maybe it should take Error|string and if given a string, wrap the string in Error for convenience.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/emmanueltouzery/prelude.ts/issues/15#issuecomment-417467607, or mute the thread https://github.com/notifications/unsubscribe-auth/AABHmlvWpFIKsSKCxByipKRpCPLBTnWjks5uWFQtgaJpZM4WTnNx .

emmanueltouzery commented 6 years ago

I pushed the change to master, this will go in 0.8.0. There are still other things I want to change before releasing 0.8.0 though, so I'm not releasing this yet. Thanks for the suggestion again!