chore: prefer AsRef<Path> to &Path #8249
Conversation
bolinfest
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
jif-oai
left a comment
jif-oai
left a comment
There was a problem hiding this comment.
Just questions on the abstraction but otherwise LGTM
codex-rs/core/src/config/service.rs
Outdated
| } | ||
|
|
||
| fn paths_match(expected: &Path, provided: &Path) -> bool { | ||
| fn paths_match<P: AsRef<Path>, Q: AsRef<Path>>(expected: P, provided: Q) -> bool { |
There was a problem hiding this comment.
OOC, why preferring this abstraction over a dyn?
OOC(2), why having both P and Q if they are the same?
There was a problem hiding this comment.
AsRef<Path> is what I see in the standard library mainly. Chat says I can do impl AsRef<Path>, so maybe I can drop P and Q.
I have both because I discovered that if you use the same letter and pass one as, say, PathBuf and the other as AbsolutePathBuf, then the typechecker fails. (I can double check...)
There was a problem hiding this comment.
Updated the PR to use impl AsRef<Path>. Maybe I'll ask Codex to try to update the codebase to use this wherever possible...
FYI, so now I have:
pub fn normalize_for_path_comparison(path: impl AsRef<Path>) -> std::io::Result<PathBuf> {
let canonical = path.as_ref().canonicalize()?;
Ok(normalize_for_wsl(canonical))
}But if it were:
pub fn normalize_for_path_comparison(path: &dyn AsRef<Path>) -> std::io::Result<PathBuf> {
let canonical = path.as_ref().canonicalize()?;
Ok(normalize_for_wsl(canonical))
}then this call site no longer compiles:
fn paths_match(a: &Path, b: &Path) -> bool {
if let (Ok(ca), Ok(cb)) = (
path_utils::normalize_for_path_comparison(a),
path_utils::normalize_for_path_comparison(b),
) {
return ca == cb;
}
a == b
}because it fails with:
error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
--> tui2/src/resume_picker.rs:675:51
|
675 | path_utils::normalize_for_path_comparison(a),
| ^ doesn't have a size known at compile-time
|
= help: within `std::path::Path`, the trait `std::marker::Sized` is not implemented for `[u8]`so I think sticking with impl AsRef<Path> is the way to go?
Over in `config_loader/macos.rs`, we were doing this complicated `mod` thing to expose one version of `load_managed_admin_config_layer()` for Mac: https://github.com/openai/codex/blob/580c59aa9af61cb4bffb5b204bd16a5dcc4bc911/codex-rs/core/src/config_loader/macos.rs#L4-L5 While exposing a trivial implementation for non-Mac: https://github.com/openai/codex/blob/580c59aa9af61cb4bffb5b204bd16a5dcc4bc911/codex-rs/core/src/config_loader/macos.rs#L110-L117 That was being used like this: https://github.com/openai/codex/blob/580c59aa9af61cb4bffb5b204bd16a5dcc4bc911/codex-rs/core/src/config_loader/layer_io.rs#L47-L48 This PR simplifies that callsite in `layer_io.rs` to just be: ```rust #[cfg(not(target_os = "macos"))] let managed_preferences = None; ``` And updates `config_loader/mod.rs` so we only pull in `macos.rs` on Mac: ```rust #[cfg(target_os = "macos")] mod macos; ``` This simplifies `macos.rs` considerably, though it looks like a big change because everything gets unindented and reformatted because we can drop the whole `mod native` thing now. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/8248). * #8251 * #8249 * __->__ #8248
This is some minor API cleanup that will make it easier to use
AbsolutePathBufin more places in a subsequent PR.