Repository navigation
Add Newtype exercise to calculate wattage. - #273
Conversation
|
I think this looks good overall. calculateWattage :: Amp -> Volt -> Watt |
Yes, I like that better. I also missed that there was already a definition of |
| import Test.NoPeeking.Solutions | ||
|
|
||
| import ChapterExamples (Amp(..), current, fromString, gcd, gcdV2, isEmpty, livesInLA, lzs, partialFunction, showPerson, showPersonV2, sortPair, takeFive, toString, unknownPerson) | ||
| import Test.NoPeeking.Solutions as Solutions | ||
| import ChapterExamples (Amp(..), current, fromString, gcd, gcdV2, isEmpty, livesInLA, lzs, partialFunction, showPerson, showPersonV2, sortPair, takeFive, toString, unknownPerson, Volt(..), Watt(..)) | ||
| import Data.Int (round) | ||
| import Data.Maybe (Maybe(Just, Nothing)) | ||
| import Data.Person (Person) | ||
| import Data.Picture (Shape(..), Picture, getCenter, origin) | ||
| import Data.Picture (Shape(..), Picture, getCenter, origin, shapeBounds) |
There was a problem hiding this comment.
Let's rebase this on the fix for #274 (in progress). That should simplify some of these import diffs. There are also some complications in this PR due to this chapter is missing the automated solutions reset flag. I'll fix that as well before rebasing this PR.
import Test.NoPeeking.Solutions -- Note to reader: Delete this line
Co-authored-by: milesfrain <milesfrain@users.noreply.github.com>
|
Let me know if you're actively rebasing this. I was going to do that now to clean-up the merge conflict I introduced. |
Co-authored-by: milesfrain <milesfrain@users.noreply.github.com>
91c27aa to
aeb5e1b
Compare
| let | ||
| (MakeWatt w) = calculateWattage (Amp 0.5) (Volt 120.0) | ||
| Assert.equal 60.0 w | ||
| Assert.equal (MakeWatt 60.0) | ||
| $ calculateWattage (Amp 0.5) (Volt 120.0) |
There was a problem hiding this comment.
I think this is a little more idomatic
There was a problem hiding this comment.
I think I tried something like that, but as I recall, the compiler complained about the lack of an Eq instance for Watt. I also thought it would be unreasonable to ask the reader to include a deriving Eq or something to their definition of Watt just to satisfy the tests, because they haven't been exposed to type classes yet.
If this works, I'm good with it.
|
Let me know if you're ready for this PR to merge. |
I'm thinking this exercise might be even more valuable to readers if we don't rely on an existing constructor, so they get to create their own. That would involve the following changes:
This could be a follow-up PR. Also, sorry if the force push on your branch causes any inconvenience. I find that makes rebases easier to manage (assuming you're not also actively working on the same branch). |
to define MakeWatt and to write calculateWattage.
|
Sorry, my builds keep failing because it seems I'm missing some step. It looks like some issue with testing the |
Other misc changes: - No need for Columb Eq and Show - Construct with Watt, rather than MakeWatt
|
This is unfortunately a bit of a tricky (and rare) situation with the book's workaround for both allowing CI on "official" solutions, while also allowing resetting solutions when the exercise involves modifying an existing ADT. This is fixed now. Awaiting your approval to merge. |
|
@milesfrain Thanks for your help! |
One simple proposal to address #255 using an extension of the existing example in the Newtype section.