Closed lbragile closed 3 years ago
Merging #72 (d0daa69) into master (a315342) will not change coverage. The diff coverage is
100.00%
.
@@ Coverage Diff @@
## master #72 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 39 39
Lines 512 509 -3
Branches 91 88 -3
=========================================
- Hits 512 509 -3
Impacted Files | Coverage Δ | |
---|---|---|
src/colorModels/hex.ts | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update a315342...d0daa69. Read the comment docs.
Hi @lbragile. Thanks for the suggestion but the approach you offered has drawbacks. It has a bigger O(N) complexity since you map
arrays. The new code also includes an extra regular expression match which is actually a super expensive operation.
The current code (which was mostly implemented by awesome @subzey) doesn't look super elegant, I agree. But it's extremely fast since it runs a single regexp and doesn't do many operations inside.
If you run npm run benchmark
in your branch you'll see that your code makes the library 2x times slower:
master: 1 608 969 ops/sec
issue-71-hex-simplifications: 931 443 ops/sec
Also, your code makes the library core 2% heavier (run npm run size
to check the size).
In other words, if code doesn't look fancy, it doesn't mean that the code is bad 😁
Changed
Note
Without
"endOfLine": "auto"
in the prettier config section ofpackage.json
, I was getting a lot of end of line errors, which is why I added this.closes #71