-
Notifications
You must be signed in to change notification settings - Fork 45
Fix SourceAnnotation ranges and refactor to std::fmt::Display
#27
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
Conversation
This PR looks awesome! Thank you so much!
for test_i26 I'd panic for now. We can improve fallibility later.
Ok, should panic
Can you mark this PR as draft until you're ready for review? I'm excited to review it :)
It seems that the coverage is not correct.
Exactly tests/fixtures/no-color/multiline_annotation.yaml.
In the report details:
thread 'test_fixtures' panicked at 'SourceAnnotation range `(23, 786)` is bigger than source length `766`', src/display_list/from_snippet.rs:190:9
So I think there are more bugs. So I will review that part of the logic.
std::fmt::Display (追記ここまで)
Should already be presentable and debb3e2 doubles performance
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.
Overall - very impressive! Thank you! The patch looks great and is definitely a significant clean up!
Several leftovers:
- the format example crashes, fix the second annotation to
(21,724)(I assume the new values are a fix to calculations) - same with the bench
- your branch seem to have broken color mode -
cargo run --example format --features color.
The changes are already there.
A detail is that the ranges are start at 0, it should start at 1.
We would have to put much more coverage.
Well but for this PR there is already enough,
Thank you! This looks awesome!
Uh oh!
There was an error while loading. Please reload this page.
In
test_i26what should be the output?panic or anything other than the label disappearing?
I would panic or even refactor to TryFrom. What you prefer
Fix #26