-
Notifications
You must be signed in to change notification settings - Fork 14k
Simplify code to generate line numbers in highlight #148171
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -343,7 +343,7 @@ struct TokenHandler<'a, 'tcx, F: Write> { | |||||
| /// We need to keep the `Class` for each element because it could contain a `Span` which is | ||||||
| /// used to generate links. | ||||||
| href_context: Option<HrefContext<'a, 'tcx>>, | ||||||
| write_line_number: fn(u32) -> String, | ||||||
| write_line_number: LineNumberKind, | ||||||
| line: u32, | ||||||
| max_lines: u32, | ||||||
| } | ||||||
|
|
@@ -355,10 +355,10 @@ impl<F: Write> std::fmt::Debug for TokenHandler<'_, '_, F> { | |||||
| } | ||||||
|
|
||||||
| impl<'a, F: Write> TokenHandler<'a, '_, F> { | ||||||
| fn handle_backline(&mut self) -> Option<String> { | ||||||
| fn handle_backline(&mut self) -> Option<impl Display + use<F>> { | ||||||
| self.line += 1; | ||||||
| if self.line < self.max_lines { | ||||||
| return Some((self.write_line_number)(self.line)); | ||||||
| return Some(self.write_line_number.render(self.line)); | ||||||
| } | ||||||
| None | ||||||
| } | ||||||
|
|
@@ -376,8 +376,7 @@ impl<'a, F: Write> TokenHandler<'a, '_, F> { | |||||
| if text == "\n" | ||||||
| && let Some(backline) = self.handle_backline() | ||||||
| { | ||||||
| self.out.write_str(&text).unwrap(); | ||||||
| self.out.write_str(&backline).unwrap(); | ||||||
| write!(self.out, "{text}{backline}").unwrap(); | ||||||
| } else { | ||||||
| self.push_token_without_backline_check(class, text, true); | ||||||
| } | ||||||
|
|
@@ -437,20 +436,29 @@ impl<F: Write> Drop for TokenHandler<'_, '_, F> { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| fn scraped_line_number(line: u32) -> String { | ||||||
| // https://developers.google.com/search/docs/crawling-indexing/robots-meta-tag#data-nosnippet-attr | ||||||
| // Do not show "1 2 3 4 5 ..." in web search results. | ||||||
| format!("<span data-nosnippet>{line}</span>") | ||||||
| } | ||||||
|
|
||||||
| fn line_number(line: u32) -> String { | ||||||
| // https://developers.google.com/search/docs/crawling-indexing/robots-meta-tag#data-nosnippet-attr | ||||||
| // Do not show "1 2 3 4 5 ..." in web search results. | ||||||
| format!("<a href=#{line} id={line} data-nosnippet>{line}</a>") | ||||||
| /// Represents line numbers to be generated as HTML. | ||||||
|
||||||
| /// Represents line numbers to be generated as HTML. | |
| /// Represents the type of line number to be generated as HTML. |
maybe?
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.
Agreed, adding it. :)
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.
The fact that we're using Cow::Owned here didn't sit right to me, so I did some digging and found out push_token_without_backline_check has overly restrictive lifetimes. It should use text: Cow<'_, str>, not text: Cow<'a, str>.
Actually, I think it could even be just text: &str, since it doesn't look like the code ever takes advantage of the Owned case and instead just uses it as impl Display, but that would require more refactoring than just changing a lifetime
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.
backline is a LineNumber, so we need to convert it to something compatible.
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 mean, I think impl Display would also work? not sure the monomorphization would be worth it, could maybe experiment in a follow-up PR.
Regardless, the lifetime still is overly restrictive, and it's hard to see that as anything but a mistake.
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.
Hum, might be worth trying in a follow-up.
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.
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 idea.