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

internal: Enforce utf8 paths #16889

Merged
merged 1 commit into from
Mar 19, 2024
Merged

internal: Enforce utf8 paths #16889

merged 1 commit into from
Mar 19, 2024

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Mar 19, 2024

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.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 19, 2024
@Veykril Veykril force-pushed the utf8-path branch 6 times, most recently from c69d7f9 to 7a18ba7 Compare March 19, 2024 13:25
@Veykril Veykril marked this pull request as ready for review March 19, 2024 14:40
@Veykril
Copy link
Member Author

Veykril commented Mar 19, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Mar 19, 2024

📌 Commit 399dbc0 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 19, 2024

⌛ Testing commit 399dbc0 with merge 4e54b4b...

@bors
Copy link
Contributor

bors commented Mar 19, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 4e54b4b to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Mar 19, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 4e54b4b to master...

@bors bors merged commit 4e54b4b into rust-lang:master Mar 19, 2024
11 checks passed
@bors
Copy link
Contributor

bors commented Mar 19, 2024

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

@Veykril Veykril deleted the utf8-path branch March 19, 2024 15:11
bors added a commit that referenced this pull request Mar 20, 2024
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 {
Copy link
Member

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)?)

bors added a commit that referenced this pull request Aug 2, 2024
internal: Remove AbsPathBuf::TryFrom impl that checks too many things at once

#16889 (comment)
lnicola pushed a commit to lnicola/rust that referenced this pull request Aug 13, 2024
internal: Remove AbsPathBuf::TryFrom impl that checks too many things at once

rust-lang/rust-analyzer#16889 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants