search

digitalbazaar / cborld

A Javascript CBOR-LD processor for web browsers and Node.js apps.
https://json-ld.github.io/cbor-ld-spec/
BSD 3-Clause "New" or "Revised" License
18 stars 12 forks source link

Updating matchAll fix from PR #18. #19

Closed davidlehn closed 3 years ago

davidlehn commented 3 years ago

Update to #18.

codecov-io commented 3 years ago

Codecov Report

Merging #19 (1b1c86b) into main (2165ec7) will increase coverage by 12.02%. The diff coverage is 40.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main      #19       +/-   ##
===========================================
+ Coverage   58.26%   70.29%   +12.02%     
===========================================
  Files          16       17        +1     
  Lines         369      377        +8     
===========================================
+ Hits          215      265       +50     
+ Misses        154      112       -42
Impacted Files Coverage Δ
lib/util.js 25.00% <25.00%> (ø)
lib/codecs/UrlCodec.js 93.93% <100.00%> (+57.57%) :arrow_up:
lib/context.js 78.12% <0.00%> (+3.12%) :arrow_up:
lib/encode.js 72.72% <0.00%> (+20.45%) :arrow_up:

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 2165ec7...1b1c86b. Read the comment docs.

davidlehn commented 3 years ago

@tplooker #18 had some issues. I tried to fix them here. The added test wasn't even running through the UrlCodec code that called matchAll. That was due to id aliases not working yet. While working around that I ran into a decode bug, so that got fixed (I think). Also turns out the match-all code is not quite like matchAll(), so had to write some code to adapt it. I think this compatibility cruft should go away sooner rather than later. At least some test coverage got added!

davidlehn commented 3 years ago

@dmitrizagidulin @msporny: It would be good to get some more eyes on that decode call dropping the multibase prefix fix. The test coverage here needs work, but seems like that would have been found elsewhere. Maybe that path wasn't used? Worth noting in case it effects other code. Tag whoever might need to know.

tplooker commented 3 years ago

@davidlehn, funny timing whilst using my patch this afternoon upstream I found the same issue withmatch-all and came up with pretty much the same solution.