-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
commented generates improper comments #114
Comments
I can't reproduce this. The following code works fine for me: const cborString = '82D818584983581C9C708538A763FF27169987A489E35057EF3CD3778C05E96F7BA9450EA201581E581C9C1722F7E446689256E1A30260F3510D558D99D0C391F2BA89CB697702451A4170CB17001A6979126C'
cbor.comment(cborString, function(error, string) {
if (error != null) {
console.error(error)
} else {
console.log(string)
}
}) As does the equivalent code: cbor.comment(cborString).then(console.log, console.error) We can argue over what the output format should be, but a) it's not standardized, and b) it's at least not wrong:
I get the same output from |
Is your argument that the commented format should dig into tag 24 and parse it as well? I'm open to that. |
Waiting to hear back from you before I cut a release with the patch above. It now generates a (still ugly, but arguably more useful):
I think to make it prettier, I'll need to get into wrapping, which will be fiddly to get exactly right, particularly with more deeply-nested inputs. |
No, I think it should not dig into the tag 24 by default, the tag itself might be of some interest for some. |
Yeah, it works fine. I think I had a character typo in what I was trying. But, I think a few things can be improved here, eg. d8 -- next 1 byte this can be printed as d8 18 for the readability, similarly This isn't required to be incorporated, you can release. |
If possible and sounds ok, can you please undo the change that digs deep inside the 24 tag by default. |
Looks like the commenter is not able to comment a bigger cbor,
This parsed until only half somewhere. |
After further investigating the depth is not coming back to 3 after the first 2 items in the second array. I have logged the depth in the commenter to debug the issue. there should be 4 items at depth 3. |
Also the commenter is not working for this valid cbor,
|
@hildjj this one looks critical |
I'll take a look. I'm going to make digging into tag 24 possible, but not the default. Setting max_depth correctly fixes a bunch of problems, so I'm looking into how to set it automatically by decoding twice -- that's a little difficult to do in the streaming approach, but I'll figure something out. |
Oh, also max_depth interferes with the same-named option on Decoder, so I'm going to rename it to |
For longer inputs, you also need to set the highwaterMark until I fix #43 in some better way. |
that was the issue proly when I tried to manually set max_depth to a larger number but no luck. Also I don't seem to be able to set highwaterMark for cbor.comment |
I wanted to fix this more completely before doing a release, but I need to get a bugfix out, and can't be bothered to do branching correctly, apparently. Apologies to myself in the future as I trigger 5.1.1. |
I tried using commenter, eg, require('cbor/lib/comented'),
commented.comment(cborString, function(error, string){
console.log(error, string);
})
This does not invoke the callback function, have tried with an options object as well.
I also tried commenting a cbor trying using cbor-cli and piping the output to a file, the comments generates are not proper and does not follow the ones as cbor.me, I tried with a couple of cbor strings, but below is a sample cbor that does not comment properly,
82D818584983581C9C708538A763FF27169987A489E35057EF3CD3778C05E96F7BA9450EA201581E581C9C1722F7E446689256E1A30260F3510D558D99D0C391F2BA89CB697702451A4170CB17001A6979126C
The text was updated successfully, but these errors were encountered: