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 Future.bichain #411

Merged
merged 1 commit into from
Feb 2, 2020
Merged

Add Future.bichain #411

merged 1 commit into from
Feb 2, 2020

Conversation

dicearr
Copy link
Contributor

@dicearr dicearr commented Jan 5, 2020

Close #332

@dicearr dicearr self-assigned this Jan 5, 2020
@codecov
Copy link

codecov bot commented Jan 5, 2020

Codecov Report

Merging #411 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #411   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          45     46    +1     
  Lines        1957   1976   +19     
=====================================
+ Hits         1957   1976   +19
Impacted Files Coverage Δ
src/bichain.js 100% <100%> (ø)
index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95fb0f3...d30bdb5. Read the comment docs.

Copy link
Member

@Avaq Avaq left a comment

Choose a reason for hiding this comment

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

Looks good so far. Things missing:

  • Documentation
  • TS Types
  • associativity and identity for bichain
  • all algebraic properties for the left branch of bichain

@dicearr
Copy link
Contributor Author

dicearr commented Jan 7, 2020

I don't know to which algebraic properties for the left branch of bichain you're referring to.

I was also thinking of adding a chain derived from bichain test in the same way map derived from bimap although, bichain is not part of the Fantasy Land spec.

README.md Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
Comment on lines 62 to 64
property('bichain left identity', _x, _fm, function (x, f){
return eq(bichain(f)(reject)(reject(x)))(f(x));
});
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the wrong interpretation of "left identity". I don't think it means "identity on the left branch", but rather it exists because for Monad, you can't test identity in a single equation. This also explains your confusion:

I don't know to which algebraic properties for the left branch of bichain you're referring to.

Once you have changed left identity to apply to the right branch, you need separate tests for testing left and right identity on the left branch.

Copy link
Member

Choose a reason for hiding this comment

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

Essentially what we're testing is that chain (resolve) is the identity function for m. So what you should be testing is that bichain (reject) (resolve) is the identity function for m.

Copy link
Member

Choose a reason for hiding this comment

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

In the chain equations, this property is verified in two steps:

left  | chain (f) (resolve (x)) = f (x)
right | chain (resolve) (m)     = m

Copy link
Member

Choose a reason for hiding this comment

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

So for bichain it should probably look like:

left  | bichain (f) (g) (reject (x))   = f (x)
left  | bichain (f) (g) (resolve (x))  = g (x)
right | bichain (reject) (resolve) (m) = m

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 understand now the difference between right and left identity. I always thought it was related to the branches. In any case, from your comments I have changed the code and hopefully it is right now.

Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand the naming of the assertions either. Maybe the idea was something like: "identity as asserted from the left side" and "identity as asserted from the right side". Maybe we can get @puffnfresh to recall how he got to the naming, and whether our interpretation of it is right.

I think of the two assertions as:

  1. "given that chain (f) (resolve (x)) is the equivalent of f (x)"; (left identity)
  2. "chain (resolve) (m) should be the equivalent of m". (right identity)

But I might be wrong. The code looks good to me now, but let's wait a bit and see if Brian will respond :)

@dicearr dicearr marked this pull request as ready for review January 8, 2020 11:01
Copy link
Member

@Avaq Avaq left a comment

Choose a reason for hiding this comment

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

I guess we merge it. It's the best we could do ;)
We can always fix it later if we become wiser.

@dicearr dicearr merged commit cc9361c into master Feb 2, 2020
@dicearr dicearr deleted the diego/bichain branch February 2, 2020 10:16
dicearr added a commit that referenced this pull request Feb 8, 2020
- #411 (d30bdb5) Add Future.bichain
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.

Add a bichain function
2 participants