-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Gracefully handle errors in CLI #12747
Conversation
25ad3bf
to
71849d3
Compare
71849d3
to
544c16f
Compare
|
dd6715a
to
a4f5494
Compare
// The base path to which all CLI arguments are relative to. | ||
let cli_base_path = { | ||
let cwd = std::env::current_dir().context("Failed to get the current working directory")?; | ||
SystemPathBuf::from_path_buf(cwd).map_err(|path| anyhow!("The current working directory '{}' contains non-unicode characters. Red Knot only supports unicode paths.", path.display()))? |
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.
Nit: s/unicode/UTF-8/
.
We have no idea if it contains unicode or not, because unicode is just a set of numeric code points, it's not an encoding. It may well be valid Unicode encoded as UTF-32, for all we know. It's just not valid UTF8-encoded text.
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'm not so sure if we're limited to UTF8 here. Path::to_string_lossly
specifically mentioned unicode and not UTF8
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.
It only handles UTF-8. (Well, on Windows it handles a weird Windows-specific variant of UTF-8, but close enough.) It's not possible to transparently and automatically handle all the various Unicode encodings; they are ambiguous with each other. You have to either assume or specify a particular encoding.
It's too bad that the rust docs are also careless with this wording. It's meaningless to say that a byte string "is Unicode" or "is not Unicode", unless by "Unicode" you mean (or assume) some specific encoding.
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.
Yeah, still not sure about this. It's true that you can only store one encoding, but you can decode between encodings. E.g. Rust converts the Windows UTF16 paths to UTF8. That means, it isn't about what unicode encoding paths use on the file system, for as long as Rust decodes the paths to UTF8 before hand.
Also, from the camino docs https://docs.rs/camino/latest/camino/
Either way. I think using Unicode here is "correct" enough.
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 see. You (and probably the rust docs too) don't want to say UTF-8 (even though the actual decode that's failing is definitely a decode from UTF-8, or the windows "wide" variant of it) because the original path on disk on windows is UTF-16 (decoded by rust and re-encoded as "wide" UTF-8 in the OsStr). So we use "Unicode" as imprecise shorthand for "the typical Unicode encoding used on your OS, ie UTF-16 on Windows and UTF-8 everywhere else." I guess maybe that's reasonable, for lack of a more precise term.
Summary
Replace the remaining
unwrap
calls in the CLI with more gracefully propagating the errors.Long term, my goal is to replace
anyhow::Error
with a more structured representation (ideally it's just another form of Diagnostic)because these errors don't look as nice as they could. But this matches Ruff's current behavior
Test Plan
Examples: