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 concat to Maybe, Either and Identity #85

Merged
merged 1 commit into from
Feb 26, 2017
Merged

Add concat to Maybe, Either and Identity #85

merged 1 commit into from
Feb 26, 2017

Conversation

evilsoft
Copy link
Owner

Lets just start with these

This PR was requested by an early tester of things. It adds concat to the simple value types and will concat inner values if they contain Semigroups of the same type.

Also there is a BIG ol breaking change: I updated isSameType to be able to compare native JS types, not just the crocks types. So things like this can be done:

  • isSameType(String, 'hello') // true
  • isSameType('goodbye', 'hello') // true
  • isSameType('goodbye', Array) // false
  • isSameType([ 'a' ], 'a') // false

@evilsoft evilsoft added this to the 0.4.0 milestone Feb 25, 2017
@coveralls
Copy link

coveralls commented Feb 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 9c118e3 on semigroup-all into 6263d10 on master.

1 similar comment
@coveralls
Copy link

coveralls commented Feb 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 9c118e3 on semigroup-all into 6263d10 on master.

Copy link
Contributor

@rstegg rstegg left a comment

Choose a reason for hiding this comment

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

found possible fix, let me know what you think

return m.map(right => {
if(!isSameType(left, right)) {
throw new TypeError(`${t}.concat: Both containers must contain Semigroups of the same type`)
}
Copy link
Contributor

@rstegg rstegg Feb 26, 2017

Choose a reason for hiding this comment

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

M(String).concat(M(Number)) or M(Array).concat(M(Number)) should work, so should it include: !isSameType(left, left.concat(right)) somehow?

Copy link
Owner Author

Choose a reason for hiding this comment

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

If I think I understand what you are saying, I think you are saying that this would not throw an Error:

const m = Maybe.of('hello')
const n = Maybe.of(3)

// will fail as Number is NOT a semigroup
m.concat(n)

if you look we are mapping on m, which is the container passed into us. So right will be a Number and it is not the same type as String, so this fails.

Now if you are saying that they are passing in the actual constructors Array and String
That would fail because Array is not a semigroup.

Is that what you mean?

Copy link
Owner Author

Choose a reason for hiding this comment

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

What would this possible fix actually fix.
Do you have a working example that shows what you found that is broken?
It is hard to pin down what you found to be broken with that description.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Or are you saying that it should not error?

Copy link
Owner Author

Choose a reason for hiding this comment

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

From gitter chat, it seems that you meant that I should be able to [ 'a' ].concat('b'). While that is true in JS, for these types of interaction I am going to go with the FL spec for semigroup.

So I do not see an error here.

Copy link
Contributor

Choose a reason for hiding this comment

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

nope i am wrong; i meant fix to a possible error, bad wording.

@evilsoft
Copy link
Owner Author

Seems legit. Merging!

@evilsoft evilsoft merged commit 42acb1e into master Feb 26, 2017
@evilsoft evilsoft deleted the semigroup-all branch February 26, 2017 19:29
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.

3 participants