Skip to content
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

Merged
merged 2 commits into from
Oct 22, 2024
Merged

Conversation

KarelPeeters
Copy link
Contributor

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:

error: title
 --> origin.txt:2:4
  |
1 | aaa
2 | bbb
3 | ccc
  | ^^^ annotation
4 | ddd
  |

Here origin.txt:2:4 is wrong, it should be origin.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.

@KarelPeeters
Copy link
Contributor Author

Ah, I missed those failing test cases, since they're disabled on windows:

fn main() {
#[cfg(not(windows))]
tryfn::Harness::new("tests/fixtures/", setup, test)
.select(["*/*.toml"])
.test();
}

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.

src/renderer/display_list.rs Outdated Show resolved Hide resolved
tests/formatter.rs Show resolved Hide resolved
src/renderer/display_list.rs Outdated Show resolved Hide resolved
@Muscraft
Copy link
Member

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!

I believe we disabled the test harness on Windows because there were issues with the test harness running on windows.

@KarelPeeters KarelPeeters force-pushed the fix-origin branch 2 times, most recently from 87c8d40 to 9e673e0 Compare October 16, 2024 21:09
@KarelPeeters
Copy link
Contributor Author

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!

@Muscraft Muscraft merged commit e41310d into rust-lang:master Oct 22, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants