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.

Sign up for GitHub

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

Jump to bottom

everything #3

Merged
merged 1 commit into from
May 26, 2018
Merged

everything #3

merged 1 commit into from
May 26, 2018

Conversation

davidchambers
Copy link
Member

@davidchambers davidchambers commented Mar 19, 2017 •
edited
Loading

Closes #1

  • specify "files" in package.json

@davidchambers davidchambers mentioned this pull request Apr 22, 2017
Merged
gabejohnson
gabejohnson reviewed Apr 30, 2017
View reviewed changes
index.js Outdated
//. > Identity['@@type']
//. 'sanctuary-either/Identity'
//. ```
Identity['@@type'] = 'sanctuary-either/Identity';
Copy link
Member

Choose a reason for hiding this comment

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

Typo on the type? I'm guessing 'sanctuary-identity/Identity'.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I'll fix this now. :)

@gabejohnson
Copy link
Member

Does this latest commit mean we're ready to merge? 🙏

This was referenced Feb 11, 2018
Merged
Open
@davidchambers
Copy link
Member Author

Does this latest commit mean we're ready to merge?

We are getting perilously close, Gabe. ;)

I'm not sure whether it's clear from my GitHub activity, but I am steadily making progress towards many of our shared goals. :)

syaiful6
syaiful6 reviewed Apr 30, 2018
View reviewed changes
README.md Outdated
- [Applicative][]
- [Chain][]
- [Monad][]
- [Alt][] (if `a` satisfies Alt)
Copy link

Choose a reason for hiding this comment

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

I believe Identity a can't depends on a here, because Alt require kind * -> * and Identity a have kind *.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the review, @syaiful6. I actually removed this line from index.js in 7a64a83, but forgot to update the readme. I'm sorry about that. I'll remove the readme from this pull request to avoid further confusion.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. There is instance (Alternative f) => Alternative (IdentityT f), but that only works because IdentityT is * -> * -> *. IdentityT List String is distinct from Identity (List String).

index.js Outdated
//. Identity (8)
//. ```
methods['fantasy-land/ap'] = function(other) {
return Z.map(other.value, this);
Copy link

Choose a reason for hiding this comment

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

better just apply this directly i guess eg Identity(other.value(this.value))

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I've made this change. :)

README.md Outdated
'Identity([1, 2, 3])'
```

[Alt]: https://github.com/fantasyland/fantasy-land/tree/v3.2.0#alt
Copy link

Choose a reason for hiding this comment

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

Same as above, i don't thik Identity can have Alt instance.

masaeedu
masaeedu requested changes Apr 30, 2018
View reviewed changes
index.js Outdated
return Identity(Z.concat(this.value, other.value));
};

//# Identity#fantasy-land/filter :: Filterable f => Identity (f a) ~> (a -> Boolean) -> Identity (f a)
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 there is a similar issue to the one with Alt here. If Identity implements Filterable, shouldn't its signature be Identity a ~> (a -> Boolean) -> Identity a (regardless of whether a is [String] or Number or Maybe Date)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I have removed Identity#fantasy-land/filter. :)

@masaeedu
Copy link
Member

I can't find anything else, LGTM :)

syaiful6
syaiful6 reviewed May 1, 2018
View reviewed changes
Copy link

@syaiful6 syaiful6 left a comment

Choose a reason for hiding this comment

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

LGTM

davidchambers
davidchambers commented May 6, 2018
View reviewed changes
index.js Outdated
var r = next(x);
while (r.tag === next) r = f(next, done, r.value).value;
return Identity(r.value);
};
Copy link
Member Author

Choose a reason for hiding this comment

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

@safareli, does this look right to you?

Copy link
Member

Choose a reason for hiding this comment

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

👍 it looks right

davidchambers
davidchambers commented May 10, 2018
View reviewed changes
index.js
//. . 'Extend ✅ ',
//. . 'Comonad ✅ ',
//. . 'Contravariant ❌ ' ]
//. ```
Copy link
Member Author

Choose a reason for hiding this comment

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

@masaeedu, please review this table.

Copy link
Member

Choose a reason for hiding this comment

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

Looks right to me (for the ones I'm familiar with at least).

Copy link
Member

Choose a reason for hiding this comment

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

This is pretty cool btw. Is it being done across all repos?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! We're using this approach in three other repositories at the moment:

This is much better than a hard-coded table we would surely forget to update. :)

@davidchambers
Copy link
Member Author

I'm about to squash the commits. The original commits can be found at a5108eb.

@davidchambers davidchambers merged commit 64227cb into master May 26, 2018
@davidchambers davidchambers deleted the davidchambers/everything branch May 26, 2018 09:08
@davidchambers davidchambers mentioned this pull request May 27, 2018
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@safareli safareli safareli left review comments

@gabejohnson gabejohnson gabejohnson left review comments

@syaiful6 syaiful6 syaiful6 left review comments

@masaeedu masaeedu masaeedu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@davidchambers @gabejohnson @masaeedu @safareli @syaiful6