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

Clean up render snippets #324

Merged
merged 7 commits into from
Jan 11, 2024
Merged

Clean up render snippets #324

merged 7 commits into from
Jan 11, 2024

Conversation

LHolten
Copy link
Contributor

@LHolten LHolten commented Dec 13, 2023

Hey, here are some local changes I made that might be useful.

In the first commit I tried to make the render_snippets function more readable and fixed what I think was a bug.
(Looks like it would not merge more than two contexts if the last context didn't overlap the first)

In the second commit I restricted finding the primary label to those in the current context.
This makes it so that the header above each snippet points to code in the snippet.
(It is especially useful for SourceCode implementations that wrap multiple files)

Example showing different headers for different snippets:
image

src/handlers/graphical.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Benjamin-L Benjamin-L left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not confident that the x_conts.line(), x_conts.line_count() -> x_line, x_line_count change is actually more readable personally, but don't feel strongly about it. It would probably also be good to add a test for the combined-context bug in tests/graphical.rs. The rest of this looks great to me.

src/handlers/graphical.rs Outdated Show resolved Hide resolved
@LHolten
Copy link
Contributor Author

LHolten commented Jan 11, 2024

I'm not confident that the x_conts.line(), x_conts.line_count() -> x_line, x_line_count change is actually more readable personally, but don't feel strongly about it.

I thought this change was necessary because I removed the vector of SpanContents. Turns out it was not necessary so I reverted the change and it is now even more readable!

The previous test would still have overlap between the first and last context because of contexts are extended with `context_lines`.
@LHolten
Copy link
Contributor Author

LHolten commented Jan 11, 2024

Also added a test for non-adjacent contexts, which shows that each context gets a header that points to the code in the context :D

miette/tests/graphical.rs

Lines 1791 to 1802 in e16326a

╭─[bad_file.rs:1:1]
1 │ source
· ───┬──
· ╰── this bit here
2 │
╰────
╭─[bad_file.rs:5:3]
4 │
5 │ text here
· ──┬─
· ╰── also this bit
╰────

Copy link
Contributor

@Benjamin-L Benjamin-L left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New version looks really good to me :)

@zkat zkat merged commit 19c2214 into zkat:main Jan 11, 2024
11 checks passed
@zkat
Copy link
Owner

zkat commented Jan 11, 2024

Thanks for this!

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.

None yet

3 participants