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

Improving error message for invalid file types #4216

Merged
merged 5 commits into from
Jun 4, 2024

Conversation

PepinhoJp
Copy link
Contributor

Closes #2486

Refactored code for loading main source file with utf-8 error detection, providing a helpful message. See examples below:

> ./typst compile typst

error: file is not valid utf-8
 = hint: a file without an extension (`.something`) is not usually a Typst file. Check if you meant to add `.typ` to its name.
> ./typst.exe compile typst.exe

error: file is not valid utf-8
 = hint: a file with the `.exe` extension is not usually a Typst file. Check if you meant to use `.typ` instead.

@MDLC01
Copy link
Contributor

MDLC01 commented May 22, 2024

I'm not convinced the second part of the hint ("Check if you meant to add .typ to its name." / "Check if you meant to use .typ instead.") is useful. Changing the file's extension won't magically make its content valid UTF-8. Otherwise, I really like this hint!

@PgBiel
Copy link
Contributor

PgBiel commented May 22, 2024

I'm not convinced the second part of the hint ("Check if you meant to add .typ to its name." / "Check if you meant to use .typ instead.") is useful. Changing the file's extension won't magically make its content valid UTF-8.

I believe the rationale for this is in the original issue, where you may accidentally (for example) use tab autocomplete towards a previously generated PDF file with the same name instead of the source file.

With that said, it could be removed for the case without an extension, or even kept just for png / svg / pdf.

@PepinhoJp
Copy link
Contributor Author

The compiler will now only suggest adding .typ if the .typ file exists. @MDLC01 thanks for your input.

Copy link
Member

@laurmaedje laurmaedje left a comment

Choose a reason for hiding this comment

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

I think the implementation could still be simplified a bit, I've left a few comments to that effect.

I now also saw why the Hint<T> for SourceResult<T> impl wasn't in place yet, but I think with a few other changes I proposed, it won't be needed anymore anyway. Sorry for sending you down the wrong path with this!

crates/typst-cli/src/compile.rs Outdated Show resolved Hide resolved
crates/typst-cli/src/compile.rs Outdated Show resolved Hide resolved
crates/typst-cli/src/compile.rs Outdated Show resolved Hide resolved
@laurmaedje laurmaedje added this pull request to the merge queue Jun 4, 2024
@laurmaedje
Copy link
Member

Thank you!

Merged via the queue into typst:main with commit d360e75 Jun 4, 2024
6 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.

Better error message when trying to compile wrong file type.
5 participants