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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add ifElse #33

Closed
wants to merge 2 commits into from
Closed

add ifElse #33

wants to merge 2 commits into from

Conversation

kutyel
Copy link
Contributor

@kutyel kutyel commented May 27, 2020

Fixes #32

I'm missing something obvious in the tests 馃, but never used fast-check before 馃槅

giogonzo
giogonzo reviewed May 28, 2020
View reviewed changes
Copy link
Owner

@giogonzo giogonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense overall. 馃憤 I left a couple of inline comments

regarding fast-check and the test, let me propose a slightly different version of it (will hopefully happen soon 馃槄 ). The gist would be to have more arbitrary values compared to "hardcoded" ones, e.g. the predicate function could be something like fc.boolean().map(constant), etc.

src/ifElse.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@kutyel
Copy link
Contributor Author

kutyel commented Jun 12, 2020

Hi @giogonzo, I applied some of your comments (the manual currying was a hell 馃槗 ).
I still want to provide the second type signature you mentioned in #32 (comment), but I was not able to trick typescript (sorry not that familiar with it's type system yet 馃槄 ).
Also, regarding the test I tried what you mention and I couldn't make it to compile, if you have a spare minute and can help get this merged I'll be glad 馃槈 (no hurry though!)

@kutyel kutyel requested a review from giogonzo June 12, 2020 10:35
@kutyel kutyel closed this Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giogonzo giogonzo Awaiting requested review from giogonzo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

implement ifElse
2 participants
@kutyel @giogonzo