-
Notifications
You must be signed in to change notification settings - Fork 126
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
Error formatting #240
Error formatting #240
Conversation
Looks pretty good so far. When you're done with this I would like to get code coverage back up to 100% before merging. I think we need some discussion about what the formatting should look like for Buffer inputs (@theqabalist?) Please use the Also not really sure about the |
I'm not entirely sure the best way to format buffers, because they are not line oriented like a lot of practical string parsing is. So showing the whole buffer is probably really noisy. Maybe something like byte offsets plus bytes in a similar manner to what is being proposed from surrounding context like:
|
are the 1050, 1051, etc the byte offsets? what is the 0xFF on the left in the gutter area? |
Yeah, those are byte offsets. I may have misinterpreted the gutter in the original formatting thing, but I assumed it was the actual value from the position, since the position had an x in it. In looking at it again, I suppose it could be the line number reference. |
Maybe output similar to
and then we can put |
I like that too, although I worry about length a bit. Like some of the messages I have parsed are like 1300 bytes, and displaying the entirety of it seems overwhelming. |
Oh, I definitely meant not the entirety of it, haha. I like how hexdump does "16 bytes = 1 line" as the equivalence. Maybe even 8 would be good? But just treating |
So the idea is we would display |
Thanks! I see two things left:
padding example
|
Thanks for the feedback, I'll look into it asap. 👍 |
Just an update, I'm still working on this 🙂 |
No rush 🙂
…On Mon, Apr 16, 2018, 02:00 Eduard Kyvenko ***@***.***> wrote:
Just an update, I'm still working on this 🙂
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#240 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAKyr5TxNAcImWlYn-pjQYti8obQH0Q3ks5tpF2dgaJpZM4TKaCm>
.
|
@halfzebra are you still working on this? no problem if you still don't have time; i might start looking into it too. |
@wavebeem Hi Brian, I just have pushed the latest implementation for string parser to support the line number padding. Looking into the binary |
src/parsimmon.js
Outdated
var showToLineIndex = | ||
lineWithErrorIndex + 3 > inputLinesLength | ||
? inputLinesLength | ||
: lineWithErrorIndex + 3; |
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.
Are -2
and +3
the number of lines of context to show before and after the error message? I think putting them in variables would help make it more clear what's going on here.
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.
Good point, will do!
var answer = Parsimmon.formatError(input, parser.parse(input)); | ||
|
||
assert.deepEqual(answer, expectation); | ||
}); |
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.
Would you mind adding test cases for 3 and 4 digit line numbers? This is looking good but it would be nice to have.
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.
Thanks for the feedback, I'll include that kind of test asap.
Coming along nicely 😄 |
what are the odds i'm going through github and see you just pushed 33 seconds ago?! i hadn't even gotten the email yet 😄 |
Finally made the first draft of the buffer parsing error formatter, please let me know if there's anything wrong with the implementation. I will put some more time into this and hopefully finish it soon 🙂 |
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.
a couple questions and one change, please :)
test/core/formatError.test.js
Outdated
[0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0], | ||
[0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0] | ||
) | ||
); |
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.
ooh, this is a cool trick to get the array formatted how you like :]
test/core/formatError.test.js
Outdated
" 8 | 0 0 0 0 0 0 0 0 0 0\n" + | ||
" 9 | 0 0 0 0 0 0 0 0 0 0\n" + | ||
" 10 | 0 0 0 0 0 0 0 0 0 0\n" + | ||
" 11 | 0 0 0 0 0 0 0 0 0 0\n" + |
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.
i think each hex byte should be written as two digits, like 00
instead of 0
so that it is easier to read
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.
Good point, I will address this asap! 👍
src/parsimmon.js
Outdated
} | ||
return { | ||
from: byteRange.from / bytesPerLine, | ||
to: byteRange.to / bytesPerLine |
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.
do you need to do like a Math.floor(from/ perLine)
here so its stays an integer? seems like this could end up with weird values
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.
It could, thanks for pointing that out! 👍
I've been looking at how some hex editors work for displaying information in a
before
after
This way it's easy to visually jump to the specific bytes without having to Also if you get tired of this I'm always happy to finish it up too, I know I can |
My favourite hobby is ruining parties by bringing up the full extent and horror of Unicode, so here goes! 🙌 Any column-sensitive formatting could be thrown off by characters that are specified as rendered at a different width, since this applies even in monospace: I'm unsure whether this list is exhaustive, and I don't know how to find out. To make matters worse, some of these are rendered differently in different terminals. For example, —but like this in alacritty (a terminal)— I've encountered terminals that render fullwidth characters as single-width, and some double-width. If some terminal uses a web browser as a renderer, who knows what it'll do. 🤷♀️ 🤷♂️ So if we want the
We'd need a fixed list of all the non-unit-width characters, Unicode codepoints, and names, which we can get from The Unicode Consortium's official listings. And then we hope they never feel like adding any more later. |
heh, well at least this won't apply to to the buffer output format :) do you know what other compilers and parsers do with this kind of input? i think it would at least be fairly edge case most of the time so not the end of the world if we don't address it yet |
@wavebeem thanks for the feedback, the byte buffer parsing error requirements are quite straightforward. I think I can get this done over the next weekend. I've never done any work with byte buffers, so it's a trial and error. I'll let you know! @anko I'm afraid you haven't ruined the party 🙂It's surely an interesting case, but I feel like in this PR I have to focus on fixing the formatting first. |
…ith error marker.
…age up to 100% by removing unreachable edge-cases.
…the additional separator.
3f773a9
to
00bbfcc
Compare
9db8b61
to
28263c0
Compare
Ready for a review! 🙂 |
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.
Just had a question on the one line where you didn't do rounding right before the one where you did.
This looks great, and thank you so much for all your work on the PR! I'm gonna take a little bit of time just to play with it before I merge it, but I should get time to look at it this weekend!
src/parsimmon.js
Outdated
return "one of " + expected.join(", "); | ||
|
||
return { | ||
from: byteRange.from / bytesPerLine, |
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.
Is there a reason this doesn't need to be rounded also?
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.
It's because byteRange.from
should always be dividable by bytesPerLine
, since it's derived from:
// Removes the reminder from `i`
var byteLineWithErrorIndex = i - (i % bytesPerLine);
var byteRange = rangeFromIndexAndOffsets(
byteLineWithErrorIndex, // <- Used to calculate `byteRange.from`
bytesBefore,
bytesAfter + bytesPerLine,
input.length
);
function rangeFromIndexAndOffsets(i, before, after, length) {
return {
// `before` and `i` are dividable by `bytesPerLine`
from: i - before > 0 ? i - before : 0,
to: i + after > length ? length : i + after
};
}
Maybe this code should be improved, because this particular place is a bit indirect.
Please let me know if you still think there's a problem with this.
I noticed a bug when the range you're looking at for a binary parser includes byte offsets of multiple digit counts:
Notice how the The bug doesn't happen with string parsers either. Would you mind adding a test to cover this use case and fixing the issue? Also, this feature is really cool! It's hard to imagine Parsimmon without this now. 😄 |
Playing with this a bit I think the problem is you forgot to multiply by 8 in this section. Changing it fixed the display issue for me: if (isBuffer(input)) {
lastLineNumberLabelLength = (lineRange.to > 0
? 8 * lineRange.to - 1
: 8 * lineRange.to
).toString(16).length;
if (lastLineNumberLabelLength < 2) {
lastLineNumberLabelLength = 2;
}
} It's a little tricky there because the "line numbers" actually need to be multiplied by 8 since there's 8 bytes per line and it's actually a byte offset |
@wavebeem thanks for the feedback 👍 I have added the test and fixed the problem! |
Fantastic, thanks! I think this is all good to go. I'll merge this, update the changelog, release to npm, and tweet about it soon. Would you like me to @ mention you in the tweet? |
Thands, that would be great! |
This PR addresses #239
I'm not using any external dependencies to add colors, because it all boils down to having supports-color included into the project and I don't feel like forcing this in any way.
I'm willinbg to work more on this to get it to the point where the users can feel a positive change in ttheir experience with the error repoting.
Please leave your feedback! 👐
Here's an example of how the error messages might look with this PR incliuded:
Node
Browser(Chrome)
Buffer error formatting
read, making it sort of feel like paragraphs