-
Notifications
You must be signed in to change notification settings - Fork 376
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
Remove no part checked. Fixes #303. #311
Conversation
Fixed strange type Removes 'No part of the value should be checked'. Adds intro section. Fixing strange typo Fixed strange type
This is nice! |
README.md
Outdated
|
||
## `forAll` | ||
|
||
[Parametric polymorphism][pp] is essential to these algebras. When the rules for any type are specified as applied to all values (`forall a b`) of the given generic types (here `a` and `b`), the rule must hold for any Javascript values unless otherwise specified. This implies, for instance, that code that behaves differently for the `null` or `undefined` value than for other values is unlikely to comply with the rule. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you wrap this paragraph at 79 characters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change “Javascript” to “JavaScript” for consistency with the rest of the document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and done.
README.md
Outdated
@@ -363,7 +371,7 @@ method takes one argument: | |||
#### `map` method | |||
|
|||
```hs | |||
map :: Functor f => f a ~> (a -> b) -> f b | |||
map :: forall a b . Functor f => f a ~> (a -> b) -> f b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PureScript appears not to include a space before the .
. I suggest we follow suit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Space is allowed in PS before the dot btw)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, allowed in Haskell as well, at least in some example code, but it's much more rare, it seems.
@@ -159,6 +159,14 @@ Identity type, for example, could provide `Id` as its type representative: | |||
If a type provides a type representative, each member of the type must have | |||
a `constructor` property which is a reference to the type representative. | |||
|
|||
|
|||
## `forAll` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might “Parametricity” be a better heading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. If someone doesn't understand this signature because of the forall
clause:
map :: forall a b. Functor f => f a ~> (a -> b) -> f b
and tries to figure out what it is, a header that matches the confusing keyword is better than a term that -- given this confusion -- is likely unknown to them also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Shall we use forall
rather than forAll
, though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely. I'll fix that tonight.
|
||
[Parametric polymorphism][pp] is essential to these algebras. When the rules | ||
for any type are specified as applied to all values (`forall a b`) of the | ||
given generic types (here `a` and `b`), the rule must hold for any JavaScript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering how can we make restrictions polymorphism implies more clear here.
What you think on something like this:(bold is for highlighting the addition):
Parametric polymorphism is essential to these algebras. When the rules
for any type are specified as applied to all values (forall a b
) of the
given generic types (herea
andb
), the rule must hold for any possible
JavaScript valueunless otherwise specified.
Since a parametrically polymorphic value does not "know" anything about the
type variables, it must behave the same regardless of its type. This is a somewhat
limiting but extremely useful property known as parametricity.
This implies, that a function should behave in same way for all values,
i.e. inspection of value is not allowed. for instance, that code that
behaves differently for thenull
orundefined
value than for other
valuesis unlikely to complycomplying with the rule.
we can steal some ideas from https://wiki.haskell.org/Polymorphism or/and link to it too (has better explanation then wikipedia)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to expand this section. These are good ideas. The "unless otherwise specified" has to do with things like f must be a binary function
or b must be same Apply as a
. In such cases, not every value is allowable.
"is unlikely to comply" was about examples such as return r === null ? Maybe.of(r) : Maybe.of(r)
. The only way these examples could work is if their differences are not observable, so perhaps we can skip it. But it was part of the motivation for this change.
@@ -552,11 +559,7 @@ method takes two arguments: | |||
1. if `f` is not a function, the behaviour of `reduce` is unspecified. | |||
2. The first argument to `f` must be the same type as `x`. | |||
3. `f` must return a value of the same type as `x`. | |||
4. No parts of `f`'s return value should be checked. | |||
|
|||
1. `x` is the initial accumulator value for the reduction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why remove this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no problem putting it back. I just didn't see it adding anything. Is there something that makes it clear that an "empty" foldable will return this value? Do you think that this text captures that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several type signatures are missing forall
. Let's use forall
everywhere it applies, not just for the methods whose descriptions currently include “no parts of […] should be checked”.
Since we were writing Haskell-esque type signatures, we were also inheriting Haskell's implicit I'm not going to veto this PR, I think people making changes is good and I support it. But, I don't think this solves the underlying problem of people writing unlawful implementations. Also, if we're now writing PureScript-esque type signatures (or Haskell-esque with GHC's map :: forall a b f. Functor f => f a ~> (a -> b) -> f b not map :: forall a b. Functor f => f a ~> (a -> b) -> f b Being inconsistent will lead to further explanations needed about how GHC's |
The goal was not to add |
@davidchambers: Rather than adding |
@CrossEye, if you don't intend to update this pull request, shall we close it? |
I intend to create a replacement, but life has gotten in the way. Hopefully soon. |
Add |
After @joneshf said that:
What 's the reason to add forall to everything? In my opinion, it generates noise in the law definitions. A section which explains that all laws are parametrically polymorphic is enough for me. I think we have to get an agreement in #303 before doing the PR. Because it seems that no part to be checked is too much opened to interpretations:
|
This avoids the "No parts of should be checked" from the individual rules and adds a
forAll
intro section to explain what that means.It is done using
forall
annotations on signatures. Could someone with deeper knowledge of H-M types double-check that I added it correctly?