-
Notifications
You must be signed in to change notification settings - Fork 36
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 origin position computation #154
Conversation
d8c868f
to
3922b7b
Compare
Ah, I missed those failing test cases, since they're disabled on windows: annotate-snippets-rs/tests/fixtures/main.rs Lines 9 to 14 in c84e388
Is there any reason for that, maybe CRLF issues? They seem to behave as expected for me when I just remove that #[cfg(not(windows))] line!
The failing test are about cases that try to point one byte past the end of the line. Previously that was indeed possible, but only in very limited situations, eg. the highlighting only worked on EOF cases anyway. See #108 (comment) for vaguely relevant discussion about this. I'm not sure what the best way to proceed here is. Is the "one past the end of the line" use case already adequately supported by just allowing users to point at the newline characters themselves? Ideally there would be some out-of-band way to signal this case, since only using byte indices for this is tricky. |
I believe we disabled the test harness on Windows because there were issues with the test harness running on windows. |
87c8d40
to
9e673e0
Compare
9e673e0
to
695e428
Compare
I think I've addressed all of the comments, and I have rebased this PR on the current master. Let me know if I can do anything else to improve this PR! |
This PR fixes an issue in the computation of the position in the origin file. When a highlighted section starts at the beginning of the line, the origin position used to point to the end of the previous line, instead of the start of the current line. This resulted in outputs like this:
Here
origin.txt:2:4
is wrong, it should beorigin.txt:3:1
, which actually matches the highlighted setting.As a drive-by fix, 21645ad fixes
EndLine
, which was used incorrectly in multiple places that happened to cancel out.