-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
internal: Enforce utf8 paths #16889
internal: Enforce utf8 paths #16889
Conversation
c69d7f9
to
7a18ba7
Compare
@bors r+ |
☀️ Test successful - checks-actions |
1 similar comment
☀️ Test successful - checks-actions |
👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request. |
fix: Fix project discovery not checking whether the `Cargo.toml` actually exists Got dropped in #16889, somehow r-a's codebase itself doesn't even run into this so I didn't see it when testing ...
Ok(AbsPathBuf(path_buf)) | ||
} | ||
} | ||
|
||
impl TryFrom<PathBuf> for AbsPathBuf { |
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 feels like this impl should now be deleted? Not it is try
for two orthogonal reasons:
- either the path is not valid utf-8
- or the path is not absolute
Better let the caller to explictily handle both of the possiblieties and do something like
AbsPathBuf::try_from(Utf8PathBuf::try_from(PathBuf)?)
internal: Remove AbsPathBuf::TryFrom impl that checks too many things at once #16889 (comment)
internal: Remove AbsPathBuf::TryFrom impl that checks too many things at once rust-lang/rust-analyzer#16889 (comment)
Cargo already requires this, and I highly doubt r-a works with non-utf8 paths generally either. This just makes dealing with paths a lot easier.