codecov[bot]
commented
7 years ago Codecov Report
Merging #386 into develop will increase coverage by
1.44%. The diff coverage is100%.
@@ Coverage Diff @@
## develop #386 +/- ##
===========================================
+ Coverage 90.21% 91.66% +1.44%
===========================================
Files 16 16
Lines 644 672 +28
===========================================
+ Hits 581 616 +35
+ Misses 63 56 -7| Impacted Files | Coverage Δ | |
|---|---|---|
| lib/lost-utility.js | 97.87% <100%> (+3.13%) |
:arrow_up: |
| lib/lost-column.js | 90.21% <0%> (+3.26%) |
:arrow_up: |
| lib/lost-row.js | 100% <0%> (+8%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 0cf2579...114f36a. Read the comment docs.
peterramsing
What kind of change is this? (Bug Fix, Feature...) Feature.
What is the current behavior (You can also link to an issue) Solves #349
What is the new behavior this introduces (if any) Adds Grid Overlay to
lost-utility.Does this introduce any breaking changes? No, it's new functionality.
Does the PR fulfill these requirements?
Other Comments I wasn't sure how to approach error handling for this, so I took a look at the discussion in #180 and it seemed that throwing
node.errorwas the way forward, so that's what I've done. I've added a helper function to test throwing of errors, again based on what was in #180. If I've gone about it in totally the wrong way, then that's ok, just let me know how you want it to happen going forward and I can update the code.One other thing I wasn't sure of is your thoughts on comments within the code. It was a bit tricky keeping track of whats going on in this, so I broke it down into steps and put a comment. If you'd rather they weren't in there, again just let me know and I'll remove them.