Skip to content

Ch6 - account for alternative solutions for Action Multiply Int #338

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

Conversation

gomain
Copy link

@gomain gomain commented May 15, 2021

As discussed in #334 and raised in #198. This PR follows this proposal.

This would void #335.

Comment on lines 183 to 186
Assert.equal 15
Assert.assert "not one of [ 15, 125 ]"
$ flip elem [ 15, 125 ]
$ act m1 a
Copy link
Member

Choose a reason for hiding this comment

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

Downside of this change is that we lose the helpful "expected x, got y" test failure messages.

Copy link
Author

Choose a reason for hiding this comment

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

A reader wouldn't know what to expect. But fixed for consistency anyway.

@gomain
Copy link
Author

gomain commented May 16, 2021

In response to #334 (comment), The additional exercise could be

(Difficult) There are actually multiple ways to implement an instance of Action Multiply Int. How many can you think of? Purescript does not allow multiple implementations of a same instance, so you will have to replace your original implementation. Hint: the test already covers 3 implementations.

@gomain gomain force-pushed the augment-test-concreat-action-multiply-int branch from 7b9ed55 to 34c89dd Compare May 16, 2021 14:43
@milesfrain
Copy link
Member

milesfrain commented May 16, 2021

Looks good. Would you like to also add the bonus exercise to the text? Thinking we should clarify to:

Hint: The test already covers 3 unique implementations.

(just added the word "unique").

And what about removing the "Does this instance satisfy the laws listed above?" lines from the exercise texts? Those lines were written before we had the TDD framework.

It might also be helpful to be a bit more specific in what we're asking for in the first exercise. Perhaps add this clarification so beginners have something more concrete to start with (similar to the phrasing of the next String exercise).

Write an instance which implements this action:
instance actionMultiplyInt :: Action Multiply Int
Hint: This action should add an integer to itself some number of times.

I think it's also reasonable to accept the act _ m = m solution, since it is technically law abiding, but up to you whether you want to include it in the tests.

```purescript
instance actionMultiplyInt :: Action Multiply Int where
  act (Multiply m) i = i / m
```

and

```purescript
import Data.Int (pow)

instance actionMultiplyInt :: Action Multiply Int where
  act (Multiply m) i = pow i m
```
@gomain gomain force-pushed the augment-test-concreat-action-multiply-int branch from 34c89dd to fc2e78f Compare May 17, 2021 07:26
@gomain
Copy link
Author

gomain commented May 17, 2021

Hint: The test already covers 3 unique implementations.

I don't think we need to stress the unique nature of implementations. We already stated multiple implementations are not allowed.

And what about removing the "Does this instance satisfy the laws listed above?"

Changed this line to an instructive "Verify that this instance satisfies the laws listed above.". This is the criterion of this exercise.

Hint: This action should add an integer to itself some number of times.

Omitting such a hint allows readers to explore implementations that don't satisfy the laws. Given the abstractness of typeclasses, a little math won't hurt.

I think it's also reasonable to accept the act _ m = m solution

I agree with concerns raised in #198, a reader may accidentally come up with this solution by just satisfying the types. There is a comment section that addresses this solution.

@gomain
Copy link
Author

gomain commented May 17, 2021

I also altered the text, adding some elaboration on the laws.

@gomain gomain force-pushed the augment-test-concreat-action-multiply-int branch 2 times, most recently from 1076ba7 to dd9128f Compare May 17, 2021 09:07
  - Elaborate on `Action` rules.
  - Make law satisfaction an istruction.
  - Add additional exercise to explore alternatives.
@gomain gomain force-pushed the augment-test-concreat-action-multiply-int branch from dd9128f to 695b288 Compare May 17, 2021 14:38
@milesfrain milesfrain merged commit 3823b3b into purescript-contrib:master May 18, 2021
@gomain gomain deleted the augment-test-concreat-action-multiply-int branch May 18, 2021 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants