Unit test: get nectar #13
Conversation
Is there a benefit to requiring these to be specified every time? Otherwise the API is more friendly if we have sensible default values.
src/subtype.js
Outdated
| if (['2_1', '2_2', '2_3'].includes(this.level_.id)) { | ||
| this.wallMap[y][x] = 0; | ||
| tile = 'null0'; | ||
| } |
There was a problem hiding this comment.
I think we can nuke this special casing!
There was a problem hiding this comment.
Are you sure? I'd love to nuke special casing, but I'm pretty sure removing this would impact studio.code.org/hoc/1 through 3
There was a problem hiding this comment.
@ryansloan is important to only show wood walls for puzzle 1, 2 & 4 of the original Hour of Code? The video currently shows the medley of wall blocks. Puzzle 3 switches to a mix of wall types, then 4 switches back to wood walls. All other puzzles are a mix of wall types.
Current:
Without the special case for 1, 2 & 4:
| if (this.defaultFlowerColor_ === 'purple' && | ||
| config.level.flowerType !== 'purpleNectarHidden') { | ||
| throw new Error(`bad flowerType for Bee: ${config.level.flowerType}`); | ||
| } |
There was a problem hiding this comment.
Is there a benefit to requiring this to be specified every time? Otherwise the API is more friendly if we have sensible default values.
There was a problem hiding this comment.
good call; this is much nicer
| pegmanY: 0, | ||
| }); | ||
|
|
||
| bee.reset(); |
There was a problem hiding this comment.
Why do I need to call reset() here before I can use the bee instance? Should we be calling reset() in the constructor, so it's ready right away?
There was a problem hiding this comment.
Hmmm, probably. I don't know whether or not there is a good reason we aren't doing it that way; odds are good there was once a good reason for it, but after all the refactors this has been through that reason might no longer apply. I definitely support looking into changing the initialization process to include reset, but I would want to do so carefully.
Codecov Report
@@ Coverage Diff @@
## master #13 +/- ##
==========================================
+ Coverage 40.48% 42.22% +1.73%
==========================================
Files 22 22
Lines 1371 1369 -2
Branches 223 223
==========================================
+ Hits 555 578 +23
+ Misses 682 663 -19
+ Partials 134 128 -6
Continue to review full report at Codecov.
|
Hamms
left a comment
Hamms
left a comment
There was a problem hiding this comment.
LGTM pending the special casing work
|
Special-casing extracted to PR #15 👍. |
3 participants
Trying to get a better understanding of this codebase 😄, and maybe start to bump the code coverage number.
I added a few suggestions in code — feel free to accept / reject.