-
Notifications
You must be signed in to change notification settings - Fork 28
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
do not destructure arguments in _ #25
Conversation
@@ -117,6 +118,7 @@ const advancePlayer = (move, player) => | |||
Right: () => ({x: player.x + 1, y: player.y}), | |||
Down: () => ({x: player.x, y: player.y + 1}), | |||
Left: () => ({x: player.x - 1, y: player.y}), | |||
_: () => (player), |
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.
Shall we drop the parens around player
?
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 like that it is consistent with the lines above, but not strongly opinionated about it.
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.
We'll let @paldepind cast the deciding vote. :)
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.
Changed my mind. Noticed that elsewhere in the docs the parens are not used, so if I really wanted to be consistent on this I should have changed it there too.
@@ -94,7 +94,7 @@ constructors. `case` can be used as a control structure for handling the | |||
different values a type can have: | |||
|
|||
```javascript | |||
var Move = Type({Up: [], Right: [], Down: [], Left: []}); | |||
var Move = Type({Up: [], Right: [], Down: [], Left: [], Jump: [], Fire: [Number]}); |
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.
What about renaming Move
to Action
? Given these two additions that sounds like a better name to me.
Thank you for the pull request! I've left a single nitpicking comment. Otherwise it looks great! |
Updated |
Thank you! I've published a new version with this fix to npm. |
do not destructure arguments in _
#24