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.

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

feat: concatAll #404

Merged
merged 10 commits into from
Feb 24, 2018
35 changes: 35 additions & 0 deletions src/concatAll.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { concat, identical, identity, pipe, when, reduce } from 'ramda';

import stubUndefined from './stubUndefined';

const nullSemigroup = { concat: identity };
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


/**
* Returns the result of concatenating all semigroups (such as arrays or strings) in passed foldable (such as an array).
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

* Returns undefined if empty foldable was passed.
*
* @func concatAll
* @memberOf RA
* @since {@link https://char0n.github.io/ramda-adjunct/2.6.0|v2.6.0}
* @category List
* @sig [[a]] -> ([a] | undefined)
Copy link
Owner

Choose a reason for hiding this comment

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

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

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

* @sig [String] -> (String | undefined)
Copy link
Owner

Choose a reason for hiding this comment

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

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

[String] -> String | Undefined

* @sig Semigroup s => Foldable s f => f -> (s | undefined)
* @param {Foldable<Semigroup>} foldable foldable with semigroups to concatenate
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

@param {Array.<Array|string>} list List containing elements that will be concatenated

* @return {Semigroup|null} concatenated semigroups, or undefined, if empty foldable was passed
Copy link
Owner

Choose a reason for hiding this comment

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

@return {Array|string|undefined} Concatenated elements

* @see {@link http://ramdajs.com/docs/#defaultTo|defaultTo}
Copy link
Owner

Choose a reason for hiding this comment

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

why defaultTo ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Owner

Choose a reason for hiding this comment

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

Please drop defaultTo and add RA.concatRight

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove or leave those in?

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

* @see {@link http://ramdajs.com/docs/#concat|concat}
* @see {@link http://ramdajs.com/docs/#unnest|unnest}
* @see {@link http://ramdajs.com/docs/#join|join}
* @example
*
* concatAll([[1], [2], [3]]); //=> [1, 2, 3]
* concatAll(['1', '2', '3']); //=> '123'
* concatAll([]); //=> undefined;
*/
const concatAll = pipe(
reduce(concat, nullSemigroup),
when(identical(nullSemigroup), stubUndefined)
);

export default concatAll;
15 changes: 15 additions & 0 deletions src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ declare namespace RamdaAdjunct {
ap<U>(fn: Apply<(t: T) => U>): Apply<U>;
}

interface Foldable<T> {
reduce<Acc>(fn: (acc: Acc, val: T) => Acc, initAcc: Acc): Acc;
}

interface Semigroup {
// https://www.typescriptlang.org/docs/handbook/advanced-types.html#polymorphic-this-types
concat(other: this): this;
}

interface Catamorphism<T> {
cata<T1>(leftFn: (v: T1) => T, rightFn: (v: T1) => T): T;
}
Expand Down Expand Up @@ -376,6 +385,12 @@ declare namespace RamdaAdjunct {
*/
ensureArray<T>(value: T | T[]): T[];

/**
Copy link
Owner

Choose a reason for hiding this comment

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

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

* 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;
Copy link
Owner

Choose a reason for hiding this comment

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

@BjornMelgaard can you review this pls ?

Copy link
Owner

Choose a reason for hiding this comment

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

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


/**
* Returns the result of concatenating the given lists or strings.
*/
Expand Down
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export { default as reduceIndexed } from './reduceIndexed';
export { default as pickIndexes } from './pickIndexes';
export { default as list } from './list';
export { default as ensureArray } from './ensureArray';
export { default as concatAll } from './concatAll';
export { default as concatRight } from './concatRight';
export { default as reduceP } from './reduceP';
export { default as reduceRightP } from './reduceRightP';
Expand Down
35 changes: 35 additions & 0 deletions test/concatAll.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { assert } from 'chai';
import { NEL, Nil } from 'monet';

import { concatAll } from '../src';
Copy link
Owner

Choose a reason for hiding this comment

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

Use following notation in tests pls.

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

Ref:

import eq from './shared/eq';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. Why tho?

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

import eq from './shared/eq';

describe('concatAll', function() {
it('should concatenate arrays', function() {
eq(concatAll([[1], [2], [3]]), [1, 2, 3]);
});

it('should concatenate strings', function() {
eq(concatAll(['1', '2', '3']), '123');
});

it('should concatenate semigroups', function() {
eq(concatAll([NEL(1), NEL(2)]), NEL(1, NEL(2, Nil)));
});

it('should returns undefined if empty foldable was passed', function() {
eq(concatAll([]), undefined);
});

it('should throw if non-foldable is passed', function() {
assert.throws(() => concatAll(null), TypeError);
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see any tests with Semigroups.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll test it on NonEmptyList

Copy link
Owner

Choose a reason for hiding this comment

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

Try I'm looking forward to see it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

});

it('should throw if foldable contains non-semigroups', function() {
Copy link
Owner

Choose a reason for hiding this comment

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

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:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

assert.throws(() => concatAll([1, 2, null, true]), TypeError);
});

it('should throw if foldable contains non-compatible semigroups', function() {
Copy link
Owner

Choose a reason for hiding this comment

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

This test is not passing on ramda 0.23.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I remove it then?

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@wojpawlik wojpawlik Feb 24, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails in 0.21 tho!

I think I'll just remove the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ramda should be testing that in R.concat anyway.

Copy link
Owner

Choose a reason for hiding this comment

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

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

assert.throws(() => concatAll(['1', [1]]), TypeError);
});
});