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

Add Semigroup concatAll intial value quiz answer #33

Conversation

vinassefranche
Copy link
Contributor

Let's discuss this quiz answer with the P.R.
I had added it in the P.R. that started the quiz answers but there were on-going discussion so I removed it. Let's start again here.

@enricopolanski 's challenge was:

Counter argument to your thesis in src/quiz-answers/semigroup-concatAll-initial-value.md, why aren't we enforcing a NonEmptyArray then?

I agree with this and, to me, this indeed would be a way to not need an initial value. I always felt like it was too much a constraint to restrict concatAll to non empty arrays but I might be wrong.
I don't see any other answer to this quiz though 😅

Copy link

@ahrjarrett ahrjarrett left a comment

Choose a reason for hiding this comment

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

Not sure how useful this is, feel free to change / use any part of it that is helpful

Comment on lines 22 to 23
Because the array of elements can be empty.
In that case, without the initial value, we would have to return `undefined` or `null` which would most probably not match the type of the Semigroup.
Copy link

@ahrjarrett ahrjarrett May 3, 2022

Choose a reason for hiding this comment

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

Although this is factually true, I think it misses the point. The question is really about the difference between a Semigroup and a Monoid.

Suggested change
Because the array of elements can be empty.
In that case, without the initial value, we would have to return `undefined` or `null` which would most probably not match the type of the Semigroup.
Because there is no way to infer from the Semigroup what to do in case of an empty list, given that there is no `empty` we can define on a Semigroup. See [Monoid's concatAll below](https://github.com/enricopolanski/functional-programming/#the-concatall-function-1) to see the difference.

@enricopolanski
Copy link
Owner

enricopolanski commented May 4, 2022

After giving it some thougths and checking with @gcanti I think that:

a) the reference to monoids should be removed, in fact the initial value here is an excuse to talk about monoids later on, but it is not required at this stage. I propose to remove it.

b) Yes, the issue is that the list might be empty, but I would avoid to use phrasing as "knowing what to do". We know what to do, it's written in the signature, and that's exactly why we are providing an initial value, so we can respect the contract and always return an A.

c) my initial challenge was why not use a NonEmptyArray<A>. Well, that's exactly what an Array<A> of any length and an initial value A are together, a NonEmptyArray<A>. Thus, my challenge was self answered by the use of initial value.

So, long story short, the answer should be about returning an A even if the array is empty, without mentioning monoids as they involve certain properties the initial value needs to respect, properties we are not requiring at this stage. The initial proposed answer was already pointing in the right direction.

@vinassefranche
Copy link
Contributor Author

@enricopolanski I've updated the answer. What do you think?

import * as S from 'fp-ts/Semigroup'
import * as NEA from 'fp-ts/NonEmptyArray'

const concatAll = <A>(S: Semigroup<A>) => (as: NEA<A>) =>
Copy link

@cdimitroulas cdimitroulas May 11, 2022

Choose a reason for hiding this comment

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

Nit: the argument here is defined as S which is shadowing the import * as S. I recommend to rename this to something else so that there's no ambiguity between the two

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 agree. I'll fix that in a future P.R. Thanks

@enricopolanski enricopolanski merged commit ca9e964 into enricopolanski:master May 11, 2022
@vinassefranche vinassefranche deleted the add-semigroup-concat-all-initial-value-quiz-answer branch May 11, 2022 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants