-
Notifications
You must be signed in to change notification settings - Fork 102
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
Result Documentation #365
Result Documentation #365
Conversation
0470586
to
b0441b8
Compare
Hey @evilsoft or @HenriqueLimas I'd love an initial set of 👀 on this. I think everything is ready for review except the |
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.
Here is a quick pass on the first few sections. Some of the comments apply to most of the other sections.
Will do another review on some more later this week.
@evilsoft Thanks for looking at this. I'll get the review items fixed up and the |
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.
Alright was able to devote a good amount of time to this review.
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.
@dalefrancis88 Looks like you got all the stuff I found in that last review.
BUT I found a few more things, but feel 💯 that we can 🚢 🇮🇹 after these things are addressed.
Thanks so much for tackling this. These Sum Types can be very brutal to document.
docs/src/pages/docs/crocks/Result.md
Outdated
ensure(isNumber) | ||
|
||
// swapWithDefault :: Result e a -> Result String Number | ||
const swapWithDefault = |
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.
Still think we need an example that transforms as it swaps instead of using two constants. Thoughts?
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.
I've written a different version, hopefully it's a bit better, lmk
Think this will do it. Amazing work on this @dalefrancis88! |
No problem, thanks for your limitless patience :) |
This is to add the documentation for
Result
per #42Not ready for review, but happy for input