feat: concatAll #404

wojpawlik · 2018-02-23T15:24:25Z

Resolves #109.

char0n

Thank you. Nice work! There are couple of issues we need to solve before merging.

char0n · 2018-02-23T16:45:25Z

src/index.d.ts

+         * Returns the result of concatenating all semigroups (such as arrays or strings) in passed foldable (such as an array).
+         * Returns null if empty foldable was passed.
+         */
+        concatAll<S extends Semigroup>(foldable: Foldable<S>): S | null;


@BjornMelgaard can you review this pls ?

char0n · 2018-02-23T16:48:55Z

src/concatAll.js

+ * @sig Semigroup s => Foldable s f => f -> (s | null)
+ * @param {Foldable<Semigroup>} foldable foldable with semigroups to concatenate
+ * @return {Semigroup|null} concatenated semigroups, or null, if empty foldable was passed
+ * @see {@link http://ramdajs.com/docs/#defaultTo|defaultTo}


why defaultTo ?

Because it's a way to get rid of the returned null.

Yes but it's just an implementation detail. Consumer of the api should not be conserned with that. Actually I'm more inclined to use undefined instead of null. I've used some ramda functions today and noticed the ramda is never returning null. Always undefined.

http://ramdajs.com/docs and search for | undefined

Agree with undefined.

Well, actually the user of it should be aware of concerned that it'll return null or undefined when it receives empty foldable.

Yes but link to defaultTo using @see doesn't do the job. Actually I think that there should only bee two @see tags with reference to R.concat and RA.concatRight. We should also add @see to concatRight that referece to RA.concatAll.

I don't think it's necessary to reference RA.concatRight.

Also, I'm of two minds about whether or not @see R.unnest and R.join, from one perspective, concatAll does a similar thing, except it has no sensible return value for empty foldable. From the other, R.unnest and concatAll have significantly different requirements about their parameters (Chain vs Foldable Semigroup).

char0n · 2018-02-23T16:50:39Z

test/concatAll.js

+    eq(concatAll(['1', '2', '3']), '123');
+  });
+
+  it('returns null if empty foldable was passed', function() {


Your tests are failing on some ramda version. Ramdas utilities become fantasy land compatible in some specific version. We have a pattern of future detection in tests.

See here

ramda-adjunct/test/notBoth.js

Line 50 in a3109fc

if (isFantasyLandSupported) {

char0n · 2018-02-23T16:51:45Z

test/concatAll.js

+  });
+
+  it('throws if non-foldable is passed', function() {
+    assert.throws(() => concatAll(null), TypeError);


I don't see any tests with Semigroups.

Use RA.Identity or monet to create couple of tests; see here

ramda-adjunct/test/notBoth.js

Line 52 in a3109fc

eq(RA.notBoth(Just(true), Just(true)), Just(false));

I think I'll test it on NonEmptyList

Try I'm looking forward to see it

char0n · 2018-02-23T16:54:46Z

src/concatAll.js

+ * @since {@link https://char0n.github.io/ramda-adjunct/2.6.0|v2.6.0}
+ * @category List
+ * @sig Semigroup s => Foldable s f => f -> (s | null)
+ * @param {Foldable<Semigroup>} foldable foldable with semigroups to concatenate


Foldable<Semigroup> is not true for all version of ramda we are supporting.
I'd go for the following just like R.concat do

@sig [a] -> [a] -> [a] @sig String -> String -> String

And add a sentence: Can also concatenate multiple members of a fantasy-land compatible semigroup.

Partly done, added the signatures, but didn't remove the previous one.

char0n · 2018-02-23T16:56:28Z

src/concatAll.js

+const nullSemigroup = { [fl.concat]: identity };
+
+/**
+ * Returns the result of concatenating all semigroups (such as arrays or strings) in passed foldable (such as an array).


Returns the result of concatenating the given lists or strings. Note: R.concat expects all items to be of the same type. It will throw an error if you concat an Array with a non-Array value. Dispatches to the concat method of the item, if present. Can also concatenate multiple members of a fantasy-land compatible semigroup.

We should also add info about returning undefined. It's missing in this description. Please do so.

char0n · 2018-02-23T17:00:02Z

test/concatAll.js

+import eq from './shared/eq';
+
+describe('concatAll', function() {
+  it('concatenates arrays', function() {


Prefer should notations; the same applies for other tests

it('should concatenate arrays'

char0n · 2018-02-23T17:01:46Z

src/concatAll.js

+import stubNull from './stubNull';
+import fl from './fantasy-land/mapping';
+
+const nullSemigroup = { [fl.concat]: identity };


I guess we'll need to change this to be compatible from ramda 0.19.1 to ramda 0.25.0

Already done. Changed back to 'concat'.

char0n · 2018-02-24T07:26:54Z

src/concatAll.js

+ * @sig Semigroup s => Foldable s f => f -> (s | undefined)
+ * @param {Foldable<Semigroup>} foldable foldable with semigroups to concatenate
+ * @return {Semigroup|null} concatenated semigroups, or undefined, if empty foldable was passed
+ * @see {@link http://ramdajs.com/docs/#defaultTo|defaultTo}


Please drop defaultTo and add RA.concatRight

Remove or leave those in?

/* * @see {@link http://ramdajs.com/docs/#unnest|unnest} * @see {@link http://ramdajs.com/docs/#join|join} */

You explained these two in prev comments and I guess it make sense to keep them.

char0n · 2018-02-24T07:39:40Z

src/concatAll.js

+
+import stubUndefined from './stubUndefined';
+
+const nullSemigroup = { concat: identity };


Null semigroup may be incorrect name for this. Shouldn't we go for something like leftIdentitySemigroup ?

Done (but not yet pushed, but it's still useful to mark it as done for me)

char0n · 2018-02-24T07:46:30Z

src/concatAll.js

+const nullSemigroup = { concat: identity };
+
+/**
+ * Returns the result of concatenating all semigroups (such as arrays or strings) in passed foldable (such as an array).


You're being too academic on this. This is not the way how Ramda documents it's api. So again I propose the following:

Returns the result of concatenating the given lists or strings. Note: RA.concatAll expects all elements to be of the same type. It will throw an error if you concat an Array with a non-Array value. Dispatches to the concat method of the preceding element, if present. Can also concatenate multiple elements of a [fantasy-land compatible semigroup](https://github.com/fantasyland/fantasy-land#semigroup). Returns undefined if empty array was passed.

Right.

Returns undefined if empty array was passed.

Should I mention R.defaultTo there somehow? "pipe the result through R.defaultTo to replace the undefined with something useful"

The best wording I came up with.

Nope we should not do that. For me it's the same as using R.find function with signature (a → Boolean) → [a] → a | undefined. They don't tell us there what to do with undefined.

Some functions just returns undefined, as an expected output. I trying to understand your reasoning but I don't see it, I am sorry. Should we also mention defaultTo when documenting R.prop function or R.nth ? I think not.

char0n · 2018-02-24T07:47:17Z

src/concatAll.js

+ * @memberOf RA
+ * @since {@link https://char0n.github.io/ramda-adjunct/2.6.0|v2.6.0}
+ * @category List
+ * @sig [[a]] -> ([a] | undefined)


Drop the redundant parens and use Undefined to denote a Category not a value

[[a]] -> [a] | Undefined

char0n · 2018-02-24T07:47:42Z

src/concatAll.js

+ * @since {@link https://char0n.github.io/ramda-adjunct/2.6.0|v2.6.0}
+ * @category List
+ * @sig [[a]] -> ([a] | undefined)
+ * @sig [String] -> (String | undefined)


Drop the redundant parens and use Undefined to denote a Category not a value

[String] -> String | Undefined

char0n · 2018-02-24T07:52:09Z

src/concatAll.js

+ * @sig [String] -> (String | undefined)
+ * @sig Semigroup s => Foldable s f => f -> (s | undefined)
+ * @param {Foldable<Semigroup>} foldable foldable with semigroups to concatenate
+ * @return {Semigroup|null} concatenated semigroups, or undefined, if empty foldable was passed


@return {Array|string|undefined} Concatenated elements

char0n · 2018-02-24T08:01:08Z

test/concatAll.js

+    assert.throws(() => concatAll([1, 2, null, true]), TypeError);
+  });
+
+  it('should throw if foldable contains non-compatible semigroups', function() {


This test is not passing on ramda 0.23.0

Should I remove it then?

We have to find out why it's not passing and do feature detection to enable/disable test depending on features R provides for us in particular version

Ramda 0.23 just doesn't throw for concating string and array, so I changed the test to try to concat array and string ;)

This fails in 0.21 tho!

I think I'll just remove the test.

Ramda should be testing that in R.concat anyway.

Just comment the test out, don't remove it. I'll play with that after merge.

char0n · 2018-02-24T08:03:30Z

test/concatAll.js

+    assert.throws(() => concatAll(null), TypeError);
+  });
+
+  it('should throw if foldable contains non-semigroups', function() {


Your test description contains branching logic. We use more proper mechanism for this.

context('when foldable contains non-semigroups', function () { specify('should throw error', function () { assert.throws(() => concatAll([1, 2, null, true]), TypeError); }); });

Ref:

ramda-adjunct/test/notBoth.js

Line 41 in a3109fc

context('when the first function returns false', function() {

Done. It looks so neat now. Thanks, I have no experience in testing.

char0n · 2018-02-24T08:05:23Z

src/index.d.ts

+         * Returns the result of concatenating all semigroups (such as arrays or strings) in passed foldable (such as an array).
+         * Returns null if empty foldable was passed.
+         */
+        concatAll<S extends Semigroup>(foldable: Foldable<S>): S | null;


concatAll<S extends Semigroup>(foldable: Foldable<S>): S | undefined;

char0n · 2018-02-24T08:05:54Z

src/index.d.ts

@@ -376,6 +385,12 @@ declare namespace RamdaAdjunct {
         */
        ensureArray<T>(value: T | T[]): T[];

+        /**


Don't forget to align this description with the one I proposed in previous comment

char0n · 2018-02-24T08:54:01Z

test/concatAll.js

+import { assert } from 'chai';
+import { NEL, Nil } from 'monet';
+
+import { concatAll } from '../src';


Use following notation in tests pls.

import * as RA from '../src';

Ref:

ramda-adjunct/test/isBoolean.js

Line 4 in 2f5173a

import eq from './shared/eq';

Will do. Why tho?

It's a convention we use through out our tests to distinguish between R and RA functions.

wojpawlik · 2018-02-24T13:09:28Z

Oh ffs, I thought CI would pass it by now

wojpawlik · 2018-02-24T13:28:07Z

Finally.

If everything's fine, merge-squash it or something, don't rebase all of the commits on master, that'd be messy.

If you don't want to merge-squash, I can rebase-squash and force-push.

guillaumearm · 2018-02-24T14:37:05Z

@GingerPlusPlus : that's fine, here is "squash and merge" for each PRs.

wojpawlik · 2018-02-24T15:18:04Z

@guillaumearm The thing is, I don't know who does it show as the creator of the merged commit

guillaumearm · 2018-02-24T15:24:15Z

@GingerPlusPlus : don't worry about that.
Your RA.omitBy commit has been squashed : c7c9c7e

feat: concatAll

dd14b4d

char0n requested changes Feb 23, 2018

View reviewed changes

wojpawlik added 4 commits February 23, 2018 22:35

test(concatAll): use "should notation"

f2e3e45

test(concatAll): add test for external semigroups

c988c99

refactor(concatAll): undefined instead of null for empty foldable

6d076d4

docs(concatAll): add simpler type signatures

366f7dc

char0n requested changes Feb 24, 2018

View reviewed changes

char0n reviewed Feb 24, 2018

View reviewed changes

wojpawlik added 4 commits February 24, 2018 12:56

refactor(concatAll): nullSemigroup -> leftIdentitySemigroup

bd8156c

test(concatAll): change a test to pass on Ramda 0.23.0

850a28b

test(concatAll)

1b52f5c

docs(concatAll)

9eec5b9

test(concatAll): commented out a problematic test

1dfc84f

char0n approved these changes Feb 24, 2018

View reviewed changes

char0n merged commit 341f3f1 into char0n:master Feb 24, 2018


		import stubUndefined from './stubUndefined';

		const nullSemigroup = { concat: identity };

feat: concatAll #404

feat: concatAll #404

Conversation

wojpawlik commented Feb 23, 2018

char0n left a 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

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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojpawlik Feb 24, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojpawlik commented Feb 24, 2018

wojpawlik commented Feb 24, 2018

guillaumearm commented Feb 24, 2018

wojpawlik commented Feb 24, 2018

guillaumearm commented Feb 24, 2018

wojpawlik Feb 24, 2018 •

edited

Loading