You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
@zkat , I'm currently looking the span reading code to fix a couple issues I've seen (incorrect line_count and a panic with specific span size). I have a few questions regarding that code and API (some potentially requiring breaking changes):
How important is Mac OS Classic line ending (\r)? I see that source_impls supports it but Mac OS Classic has been dead for more than 20 years, and even Rust does not support that type of ending for its own lines() functions. If only Unix and Windows types are supported, the code can be simplified (i.e. \r can just be ignored, and it's enough to look for \n), and that can help simplify the code. [Edit: It looks like its already not supported. GraphicalReportHandler::get_lines does not consider single \r as newlines.]
If the SpanContents ends with a new line (e.g. "foo\n"), does that count as 1 or 2 lines? I can't use source_impls.rs to figure it out because it returns a line count from the beginning of the source instead of that of the span (one of the bug I'm fixing). Typically for editors and utilities (e.g. wc), that should be 2, but then that could be an issue with the way "lines after" context is done (see next point).
Does the "lines after" include the line ending or not? I.e. should a span be "foo\nbar\n" or "foo\nbar"? Currently, it's the former. But this could be an issue depending on the previous point (i.e. is "foo\nbar\n" 2 or 3 lines?).
And if one wants to be pedantic, a "newline" character that does not count as a "new line" is odd 😛.
If read_span is called with zero before/after lines, is it supposed to read just the span? Or should it read whole lines containing the span? source_impls does the former. But that means that if there are 0 context lines, only partial lines are displayed, which gets even weirder if the span covers multiple lines.
I'm thinking that for specifying the number of lines, it would be better to have a Option<usize>, where None mean "just the span", and Some(0) means the whole line, and Some(!0) means extra context lines.
reacted with thumbs up emoji reacted with thumbs down emoji reacted with laugh emoji reacted with hooray emoji reacted with confused emoji reacted with heart emoji reacted with rocket emoji reacted with eyes emoji
-
@zkat , I'm currently looking the span reading code to fix a couple issues I've seen (incorrect
line_count
and a panic with specific span size). I have a few questions regarding that code and API (some potentially requiring breaking changes):How important is Mac OS Classic line ending (\r
)? I see thatsource_impls
supports it but Mac OS Classic has been dead for more than 20 years, and even Rust does not support that type of ending for its ownlines()
functions. If only Unix and Windows types are supported, the code can be simplified (i.e.\r
can just be ignored, and it's enough to look for\n
), and that can help simplify the code.[Edit: It looks like its already not supported.
GraphicalReportHandler::get_lines
does not consider single\r
as newlines.]SpanContents
ends with a new line (e.g. "foo\n"), does that count as 1 or 2 lines? I can't usesource_impls.rs
to figure it out because it returns a line count from the beginning of the source instead of that of the span (one of the bug I'm fixing). Typically for editors and utilities (e.g.wc
), that should be 2, but then that could be an issue with the way "lines after" context is done (see next point).And if one wants to be pedantic, a "newline" character that does not count as a "new line" is odd 😛.
read_span
is called with zero before/after lines, is it supposed to read just the span? Or should it read whole lines containing the span?source_impls
does the former. But that means that if there are 0 context lines, only partial lines are displayed, which gets even weirder if the span covers multiple lines.I'm thinking that for specifying the number of lines, it would be better to have a
Option<usize>
, whereNone
mean "just the span", andSome(0)
means the whole line, andSome(!0)
means extra context lines.Beta Was this translation helpful? Give feedback.
All reactions