Skip to content

[email protected] #475

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

Merged
merged 1 commit into from
Feb 10, 2018
Merged

[email protected] #475

merged 1 commit into from
Feb 10, 2018

Conversation

davidchambers
Copy link
Member

@davidchambers davidchambers commented Jan 9, 2018

index.js Outdated
//# filter :: Filterable f => (a -> Boolean) -> f a -> f a
//.
//. Curried version of [`Z.filter`][]. Discards every element of the given
//. structure which does not satisfy the predicate.
Copy link
Member

Choose a reason for hiding this comment

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

Anywhere in the docs you're currently using "structure" you could be more specific by replacing it by its type class(es).

Discards every element of the given Filterable which does not satisfy the predicate.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true, although the approach only works when there is exactly one constraint on the structure. Admittedly, this is quite often the case.

On the website, constraints in type signatures are hyperlinked, reducing the importance of links to type classes in descriptions.

I consider the above “defences” quite weak. Do you find either of them compelling, Aldwin, or do you think we should replace “structure” in contexts in which it is possible to do so?

Copy link
Member

@Avaq Avaq Jan 11, 2018

Choose a reason for hiding this comment

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

the approach only works when there is exactly one constraint on the structure

In theory you could use terms like Filterable Monoid and Monadic Semigroup when having multiple constraints on one type variable, and terms like Functor of Foldable when talking about constraints on layered structures. In practice, I'm not sure if this will make things more clear.

Takes a Setoidal value and a Foldable of Setoids and returns true iff the value
is an element of the Foldable.

That starts to look like we're communicating with another species.

Copy link
Member

Choose a reason for hiding this comment

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

The context for the description is already provided by the function signature itself. Perhaps we could just omit "of the given structure".

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps we could just omit "of the given structure".

@davidchambers
Copy link
Member Author

I just pushed some significant changes to this pull request. I'd appreciate another review from you, @gabejohnson, or from another member of the community. :)

index.js Outdated
//# takeWhile :: Filterable f => (a -> Boolean) -> f a -> f a
//.
//. Curried version of [`Z.takeWhile`][]. Discards the first inner value
//. which does not satisfy the predicate, and all subsequent inner values.
Copy link
Member

Choose a reason for hiding this comment

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

This description isn't consistent with the previous two. Perhaps replace "inner value" with "element"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent suggestion! I love these small improvements. :)

@davidchambers davidchambers force-pushed the davidchambers/type-classes branch from 94a9b3a to 55cac19 Compare February 10, 2018 23:05
@davidchambers davidchambers force-pushed the davidchambers/type-classes branch from 55cac19 to 72a3f19 Compare February 10, 2018 23:07
@davidchambers davidchambers merged commit 5f6bbde into master Feb 10, 2018
@davidchambers davidchambers deleted the davidchambers/type-classes branch February 10, 2018 23:23
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