Sanctuary >0.15.0 compatibility and general improvements #8

Gipphe · 2018-10-05T08:52:16Z

Summary:

sanctuary-def must be passed to create in the configuration object.
All defined functions, as well as all type constructors, must be manually curried, or otherwise support currying.
Update babel to version 7.
Update chai and mocha.
Replace ramda with sanctuary and some single-function packages.
Replace ramda-fantasy's Reader with own implementation that is fantasy-land compatible.
Update eslint-config-airbnb-base to 13.1.0, updating its peer dependencies to match.

Closes #7

davidchambers · 2018-10-05T09:40:20Z

src/Reader.js

+Reader.ask = Reader (S.I);
+
+Reader.prototype.toString = Reader.prototype[$$show] = function show_() {
+  return 'Reader(' + show (this.run) + ')';


Should we use Reader (...) to match Sanctuary style?

You mean add a space between Reader and the opening parens?

To be more specific, change it to Reader (' + show (this.run) + ')';?

Call be impatient, but I just made the change I assumed you meant.

Gipphe · 2018-10-05T11:55:14Z

Added tests for Reader. Forgot all about those for Reader in particular, initially. Sorry about the repeated "Gipphe added some commits" with the rebases.

nkrkv · 2018-10-05T13:24:40Z

I suggest bumping version to 4.0.0 in package.json (will publish immediately after the merge). Also, a short note about the changes in the “Changelog” section of README.md is much desired.

davidchambers · 2018-10-05T13:32:54Z

I suggest bumping version to 4.0.0 in package.json (will publish immediately after the merge).

This should be part of an automated release process. Would you like me to submit a pull request to integrate xyz?

evgenykochetkov · 2018-10-05T15:03:38Z

While testing this on XOD codebase, I found an interesting thing about HOFs that deal with functions with 2+ arguments.

We parse ... -> (String -> Number -> Boolean) -> ... as ..., $.Function ([$.String, $.Number, $.Boolean]), ...:

const {types} = resolve ($) ([]) ($.env) ('foo :: String -> (String -> Number -> Boolean) -> String');
const lists = S.zip (types) ([$.String, $.Function ([$.String, $.Number, $.Boolean]), $.String]);
assertTypePairs (lists); // passes

But since $.Function behaviour did not change from >0.15 (sanctuary-js/sanctuary-def#179 (comment)) it is turned into something like ... -> ((String, Number) -> Boolean) -> ...:

const signature = 'foo :: a -> (a -> a -> Boolean) -> a';

const foo = def
  (signature)
  (() => 'implementation does not matter for this example');

assert.equal (foo.toString (), signature);
// + expected - actual
// -foo :: a -> ((a, a) -> Boolean) -> a
// +foo :: a -> (a -> a -> Boolean) -> a

Gipphe · 2018-10-05T15:32:54Z

@evgenykochetkov

But since $.Function behaviour did not change from >0.15 (sanctuary-js/sanctuary-def#179 (comment)) it is turned into something like ... -> ((String, Number) -> Boolean) -> ...:
const signature = 'foo :: a -> (a -> a -> Boolean) -> a';

const foo = def
  (signature)
  (() => 'implementation does not matter for this example');

assert.equal (foo.toString (), signature);
// + expected - actual
// -foo :: a -> ((a, a) -> Boolean) -> a
// +foo :: a -> (a -> a -> Boolean) -> a

Forgot about how $.Function types out a non-curried function... Meaning (a -> b -> c) -> d should be

[$.Function ([a, $.Function ([b, c])]), d]

instead of

[$.Function ([a, b, c]), d]

Shouldn't be that hard (right?)

nkrkv · 2018-10-05T15:33:46Z

Would you like me to submit a pull request to integrate xyz?

Yes, that would be very nice.

In this case, @Gipphe, please do not bump the version

evgenykochetkov · 2018-10-05T15:39:21Z

Forgot about how $.Function types out a non-curried function... Meaning (a -> b -> c) -> d should be
[$.Function ([a, $.Function ([b, c])]), d]
instead of
[$.Function ([a, b, c]), d]

Yes

Gipphe · 2018-10-05T16:15:22Z

@evgenykochetkov Done.

Gipphe · 2018-10-05T16:18:53Z

It now supports type signatures like foo :: (a -> b -> c) -> d as well as bar :: a -> (b -> c -> d) as you'd expect.

src/signature.js

davidchambers · 2018-10-05T20:58:03Z

src/signature.js

+const firstChild = S.pipe ([
+  children,
+  S.head,
+  unchecked.fromMaybe (undefined),


Why return undefined rather than S.Nothing?

At the time I wrote this function, I cared more to copy the dependent-upon behavior of R.compose(R.head, children), which was the previous implementation. Now that tests are passing, I can safely redo it.

Actually, this function will never receive an object that has an empty children property, so we can go even simpler and do with xs => xs[0].

Sounds good. You should update the type signature accordingly:

-// firstChild :: { children :: Array a } -> a +// firstChild :: { children :: NonEmpty (Array a) } -> a

I think this is the clearest implementation:

const firstChild = r => r.children[0];

I also suggest aligning the type signature with the identifier on the subsequent line:

// firstChild :: { children :: NonEmpty (Array a) } -> a const firstChild = r => r.children[0];

I need to update a lot of the type signatures I see now, upon further inspection. This little function's isn't the only one. Aligning the type signatures is a bit of a minor thing, but I am inclined to agree that it looks better that way.

src/signature.js

Gipphe · 2018-10-05T23:35:46Z

There are a lot of S.unchecked uses throughout signature.js, mostly because sanctuary extracts the type of Reader incorrectly. This could be remedied by actually defining a type for Reader, which we then pass to sanctuary's create. Or, and this is a question aimed more at @nkrkv, the original author: why are we using Reader at all? I haven't been around in the FP world long enough to see all the benefits a monad like Reader might bring to the table, so please do enlighten me if I am missing something obvious.

nkrkv · 2018-10-08T09:39:59Z

Or, and this is a question aimed more at @nkrkv, the original author: why are we using Reader at all

I used it at the peak of my craziness while getting familiar with FP on practice. It is actually an over-engineering legacy. It is absolutely OK to get rid of it.

Gipphe · 2018-10-08T10:34:20Z

I used it at the peak of my craziness while getting familiar with FP on practice. It is actually an over-engineering legacy. It is absolutely OK to get rid of it.

Am still struggling with over-engineering myself, so I feel ya.

BREAKING CHANGE: no longer works with regular multi-arity function application. Requires functions to be curried, either manually or with the help of functions like `ramda`'s `curry` and `curryN`, or `sanctuary`'s own curry helpers, `curry2`-`curry5`.

Multi-arity higher order functions are types as ```javascript [ $.Function ([ a, $.Function ([ b, c ]), ]), d ] ``` instead of ```javascript [ $.Function ([ a, b, c ]), d ] ```

nkrkv

👍

nkrkv requested review from nkrkv and evgenykochetkov October 5, 2018 09:17

davidchambers reviewed Oct 5, 2018

View reviewed changes

Gipphe force-pushed the master branch 3 times, most recently from 00f7707 to cc4129f Compare October 5, 2018 11:53

Gipphe force-pushed the master branch from 6502150 to 953ede4 Compare October 5, 2018 16:17

davidchambers reviewed Oct 5, 2018

View reviewed changes

src/signature.js Outdated Show resolved Hide resolved

davidchambers reviewed Oct 5, 2018

View reviewed changes

src/signature.js Outdated Show resolved Hide resolved

davidchambers reviewed Oct 5, 2018

View reviewed changes

src/signature.js Outdated Show resolved Hide resolved

Gipphe force-pushed the master branch 4 times, most recently from 0f6d17d to 50a505b Compare October 5, 2018 23:34

Gipphe force-pushed the master branch from 50a505b to 2a27d88 Compare October 7, 2018 01:21

Gipphe closed this Oct 8, 2018

Gipphe force-pushed the master branch from 2a27d88 to 3f9ea9c Compare October 8, 2018 10:36

Gipphe reopened this Oct 8, 2018

Gipphe added 7 commits October 8, 2018 12:39

build: update babel to version 7

118b5b9

style: update eslint-config-airbnb-base to 13.1.0

8c601e7

refactor: replace ramda-fantasy with own Reader

28e1468

chore: update chai to ^4.2.0 and mocha to ^5.2.0

def7e98

docs: tidy up README

7836219

fix: type multi-arity HOF

60a909d

Multi-arity higher order functions are types as ```javascript [ $.Function ([ a, $.Function ([ b, c ]), ]), d ] ``` instead of ```javascript [ $.Function ([ a, b, c ]), d ] ```

Gipphe force-pushed the master branch from 2a27d88 to 60a909d Compare October 8, 2018 10:45

nkrkv approved these changes Oct 8, 2018

View reviewed changes

nkrkv merged commit 81bec3e into xodio:master Oct 8, 2018

danielo515 mentioned this pull request Oct 9, 2018

Is a good idea to use lodash sub-packages ? #10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sanctuary >0.15.0 compatibility and general improvements #8

Sanctuary >0.15.0 compatibility and general improvements #8

Gipphe commented Oct 5, 2018 •

edited

Loading

davidchambers Oct 5, 2018

Gipphe Oct 5, 2018

Gipphe Oct 5, 2018

Gipphe Oct 5, 2018

davidchambers Oct 5, 2018

Gipphe commented Oct 5, 2018

nkrkv commented Oct 5, 2018

davidchambers commented Oct 5, 2018

evgenykochetkov commented Oct 5, 2018 •

edited

Loading

Gipphe commented Oct 5, 2018

nkrkv commented Oct 5, 2018

evgenykochetkov commented Oct 5, 2018

Gipphe commented Oct 5, 2018

Gipphe commented Oct 5, 2018

davidchambers Oct 5, 2018

Gipphe Oct 5, 2018

Gipphe Oct 5, 2018

davidchambers Oct 5, 2018

Gipphe Oct 6, 2018

Gipphe commented Oct 5, 2018

nkrkv commented Oct 8, 2018

Gipphe commented Oct 8, 2018

nkrkv left a comment

Sanctuary >0.15.0 compatibility and general improvements #8

Sanctuary >0.15.0 compatibility and general improvements #8

Conversation

Gipphe commented Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gipphe commented Oct 5, 2018

nkrkv commented Oct 5, 2018

davidchambers commented Oct 5, 2018

evgenykochetkov commented Oct 5, 2018 • edited Loading

Gipphe commented Oct 5, 2018

nkrkv commented Oct 5, 2018

evgenykochetkov commented Oct 5, 2018

Gipphe commented Oct 5, 2018

Gipphe commented Oct 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gipphe commented Oct 5, 2018

nkrkv commented Oct 8, 2018

Gipphe commented Oct 8, 2018

nkrkv left a comment

Choose a reason for hiding this comment

Gipphe commented Oct 5, 2018 •

edited

Loading

evgenykochetkov commented Oct 5, 2018 •

edited

Loading