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 fantasy-land 2.0 compat namespaced methods #86

Merged
merged 7 commits into from Nov 12, 2016
Merged

Conversation

briancavalier
Copy link
Owner

@briancavalier briancavalier commented Oct 27, 2016

This is not complete, but wanted to open it for discussion. Inspired by @bergus' comment here, it was trivial to add fantasy-land 2.0 compat, while maintaining back compat with current creed 1.x.

If we want to go this route, then there's a bit more todo:

  • Update docs
  • Merge Implement bifunctor #34, then update this PR to add namespaced bifunctor
  • Add tests to verify fl namespace methods. Not sure the best way to do this, since we're already testing FL laws. Perhaps this is the right time to use the official fl laws test suite

@coveralls
Copy link

coveralls commented Oct 27, 2016

Coverage Status

Coverage decreased (-0.5%) to 99.491% when pulling 04e24b8 on add-fl-2.0 into a006e6e on master.

@bergus
Copy link
Contributor

bergus commented Oct 27, 2016

LGTM.

@coveralls
Copy link

coveralls commented Oct 31, 2016

Coverage Status

Coverage decreased (-0.8%) to 99.241% when pulling 4434b0d on add-fl-2.0 into a006e6e on master.

@briancavalier briancavalier mentioned this pull request Oct 31, 2016
2 tasks
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 99.241% when pulling 4746514 on add-fl-2.0 into a006e6e on master.

1 similar comment
@coveralls
Copy link

coveralls commented Oct 31, 2016

Coverage Status

Coverage decreased (-0.8%) to 99.241% when pulling 4746514 on add-fl-2.0 into a006e6e on master.

@coveralls
Copy link

coveralls commented Oct 31, 2016

Coverage Status

Coverage decreased (-0.8%) to 99.241% when pulling 517d451 on add-fl-2.0 into a006e6e on master.

@briancavalier
Copy link
Owner Author

It's not clear to me why coverage isn't 100% after 4746514, and the html coverage isn't any help :/ If you look at the diff with master, only the new namespaced methods have been added. Since they just just immediately call existing code (which was already at 100% coverage), they should be fully covered simply by calling them at least once somewhere in the test suite, which is what 4746514 does.

If you look back at this coveralls comment, which is just before 4746514, then commit where I added the laws. Then, look at this one, after that commit, the coverage percentage is exactly the same. IOW, the fantasy land laws, somehow, are simply not being counted by coveralls.

I'll try a test where I explicitly call one of the new methods in a test directly, rather than letting a fantasyland law call it, and see if that helps.

@briancavalier
Copy link
Owner Author

That seems to be the problem: istanbul isn't reporting code covered by the fantasy land laws. (perhaps since they are outside of the dirs being instrumented?)

Here's what local istanbul reports for me in this PR branch:

Statements   : 99.27% ( 813/819 )
Branches     : 89.96% ( 206/229 )
Functions    : 97.38% ( 223/229 )
Lines        : 99.24% ( 784/790 )

And here's what it reports for me when I use a hand-coded Functor identity test instead of delegating to fantasy-land/laws/functor.

Statements   : 99.39% ( 814/819 )
Branches     : 89.96% ( 206/229 )
Functions    : 97.82% ( 224/229 )
Lines        : 99.37% ( 785/790 )

I guess an alternative is to write our own simple set of tests for the FL namespaced methods in addition to using the FL test suite. I was hoping just to rely on the FL suite, tho.

@coveralls
Copy link

coveralls commented Nov 3, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 45d71db on add-fl-2.0 into a006e6e on master.

@briancavalier
Copy link
Owner Author

Haha, lovely. Fantasy Land's laws node modules require a version of node that supports destructuring. Since node 6 is LTS now, we could drop node 4 & 5.

@briancavalier briancavalier mentioned this pull request Nov 3, 2016
@briancavalier
Copy link
Owner Author

See #87

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e393a58 on add-fl-2.0 into e2f5510 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 1eb3e8f on add-fl-2.0 into e2f5510 on master.

@briancavalier
Copy link
Owner Author

Now that FL 2.1 is released, it might be more correct to implement Alt (currently concat, i.e. race) and Plus (currently never), rather than Semigroup and Monoid. Since Monoid should probably concat the values within the promises, rather than racing.

We could still leave concat for backcompat in 1.x.

Thoughts?

@briancavalier briancavalier merged commit 4472a7b into master Nov 12, 2016
@briancavalier briancavalier deleted the add-fl-2.0 branch November 12, 2016 02:25
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

3 participants