-
Notifications
You must be signed in to change notification settings - Fork 95
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
[email protected] compatibility #131
base: master
Are you sure you want to change the base?
Conversation
I'm using ramda-fantasy in tests and currently this is what i'm doing. |
this looks fine to me, although it does make me wonder again what the purpose of two separate libs (S & RF) is. See #105 |
@@ -125,5 +125,10 @@ Either.Left = function(value) { | |||
return new _Left(value); | |||
}; | |||
|
|||
require('./internal/fl-patch.js')([ |
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.
For consistency let's drop .js
from the path.
return Object.keys(fl).forEach(function(key) { | ||
if (typeof obj[key] === 'function') { | ||
if (key === 'ap') { | ||
obj[fl[key]] = fixedAp; |
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 doesn't feel right to me. What would be the consequences of removing this exception?
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.
removing this means we should revert ap
of all structures
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.
Change the way the existing ap
methods behave, do you mean? I think we should do so. Having an ap
method with the old behaviour is confusing.
@@ -30,7 +30,8 @@ | |||
"test": "mocha" | |||
}, | |||
"dependencies": { | |||
"ramda": ">=0.15.0" | |||
"ramda": ">=0.15.0", | |||
"sanctuary-type-classes": "0.1.0" |
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 just published v0.2.0. :)
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.
congrats
return stepFn(Tuple.fst(t))._run(Tuple.snd(t)).chain(function (t_) { | ||
return M.of(Tuple.fst(t_).bimap( | ||
return Z.chain(function (t_) { | ||
return Z.of(M,Z.bimap( |
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.
s/M,Z/M, Z/
We might as well go straight to |
@@ -30,7 +30,8 @@ | |||
"test": "mocha" | |||
}, | |||
"dependencies": { | |||
"ramda": ">=0.15.0" | |||
"ramda": ">=0.15.0", | |||
"sanctuary-type-classes": "0.2.0" |
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 just published v0.3.0. :)
Do we want this PR to land before we wrap up with a final release? |
Yes, I think so. |
|
It's breaking change as
sanctuary-type-classes
dispatches to namespaced methods so if someone was usingramda-fantasy
with some other non[email protected]
compatible lib it will break.But it's not too breaking as every method is as it was before they are just copied to used to define namespaced versions in
fl-patch
(it also flipsap
)