Conversation
PgBiel
left a comment
There was a problem hiding this comment.
This is a nice change! I've left some initial observations.
|
Ok, I fixed all of the current code review items ;) |
PgBiel
left a comment
There was a problem hiding this comment.
Thanks, it's all looking much better :)
|
|
||
| if is_newline(c) { | ||
| if was_cr && c == '\n' { | ||
| was_cr = false; |
There was a problem hiding this comment.
can't we instead advance s in here and skip the was_cr state? (note: possibly not relevant if we do the lines thing below.)
| format: ImageFormat, | ||
| alt: Option<EcoString>, | ||
| ) -> StrResult<Image> { | ||
| span: Span, |
There was a problem hiding this comment.
As discussed on Discord, we should avoid passing spans and file IDs into memoized functions if they are only used for formatting the error message. Rather, we'd have a return type that is not SourceResult and turn it into a SourceResult using span & ID outside. (Adding this to the PR discussion for completeness' sake.)
|
|
||
| let text = String::from_utf8_lossy(&bytes); | ||
|
|
||
| let mut line = 1; |
There was a problem hiding this comment.
This looks very similar to the Span code. The Source type can already do it (and faster than this here!) with a lines vector. Maybe we could even turn that into a reusable Lines type that is not tied to Typst source files, but can also handle its in-place edits! Especially in the context of the web app, we'll also need the UTF-16 conversion methods to be able to make use of these changes.
We could even consider memoizing the output via lines(bytes: &Bytes) -> Arc<Lines> (right now many errors in a long binary file would result in technically O(n^2) runtime).
This is a bit of larger thing and I know you're busy, so don't feel forced to work on this. Maybe I'd pick it up otherwise.
| } else { | ||
| format!("`{}`", self.test.source.text()[range.clone()].replace('\n', "\\n")) | ||
| let path = system_path(file).unwrap(); | ||
| let bytes = read(&path).unwrap(); |
There was a problem hiding this comment.
Why not use self.world.file(file) here (and below)? Then we can revert the changes in world.rs.
|
@laurmaedje I didn't have time to finish it last week in the end, thanks for the review, I come back from a work trip on Thursday evening so I'll try and get to it maybe on Friday :) |
|
Sounds good, no rush |
|
I'll close this due to inactivity. No worries about it, I fully understand that you're quite busy! :) We can always go back and pick this back up or use it as a base for a second attempt. |
This adds spans for all errors that occur in files outside of Typst's source files (i.e SVG images, external JSON, etc.)
This required quite a laundry list of changes, including to the test framework most notably:
// Error: "/path/to/external/file" 3-5 ...// Error: "/path/to/external/file" #3-#5-> this is needed for handling of edge cases where errors occur on line breaks, a bit annoying, if you find a workaround, let me knowAnd then changes in all of the image decoding (for SVG), and all of the loaders (
csv,json,xml).There are likely stuff left to do and discussions to be had, but now I am going to bed.