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
fix(reduce-idents): improve encode #862
Conversation
Codecov Report
@@ Coverage Diff @@
## master #862 +/- ##
==========================================
+ Coverage 97.66% 97.66% +<.01%
==========================================
Files 111 111
Lines 3338 3339 +1
Branches 1011 1011
==========================================
+ Hits 3260 3261 +1
Misses 71 71
Partials 7 7
Continue to review full report at Codecov.
|
/cc @ben-eb can you look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I think out of the scope of this PR but I'm not a fan of how we test the encoder here. Do you have any suggestions? One I was thinking about recently was having some kind of test "oracle" which would return specific character pairs to capture the edge cases, then test the real implementation against it. Something like:
const oracle = (val, num) => ({ 51: 'Z', 52: 'aa' }[num]);
...
expect(encode(null, num)).toBe(oracle(null, num));
What do you think? Possibly with a few more cases in there? 🙂
Thanks, I totally agree with you. I could add more edge cases according to the encoder spec with hardcode later. —- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks! 💯
fix #853