-
-
Notifications
You must be signed in to change notification settings - Fork 87
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
Implement nand #952
Implement nand #952
Conversation
cb90bd4
to
9ab183c
Compare
I hopefully addressed all your feedback now! Thank you for the super quick response |
src/index.d.ts
Outdated
@@ -979,6 +979,11 @@ declare namespace RamdaAdjunct { | |||
*/ | |||
neither(firstPredicate: Function, secondPredicate: Function): Function; | |||
|
|||
/** | |||
* Returns `true` if both arguments are falsy, otherwise `false`. |
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.
This description is not sync with description in nand.js
test/nand.js
Outdated
}); | ||
|
||
it('should support currying', function() { | ||
const trueNand = nand(true); |
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.
Pls import RA in following way import * as RA from '../src';
can we simplify into ?
it('should support currying', function() {
eq(RA.nand(true, true), false);
eq(RA.nand(true)(true), false);
});
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 provided last to comments, after processing them we're good to merge. Sorry for a lot of comments, but we're trying to maintain consistency and quality of the codebase very hard ;]
No worries, I totally hear you about opensource projects. Keeping things consistent can be really hard. |
I've pushed up a change which hopefully addresses all your feedback. I hope rebasing the PRs is okay. |
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.
Very nice! Let's wait for tests and it's ready for a merge. Thank you again.
Tries to solve #237
Note: While committing it, eslint-plugin-ramda had a bug, see ramda/eslint-plugin-ramda#26, so I disabled this specific eslint rule in the configuration. Maybe I'm missing something.