feat: add Sys trait for swapping out system #109
Conversation
|
I like this but can we adjust the presentation a bit? I'd consider this a feature for advanced users. So we should move the README example into the generated rustdocs. Additionally let's not flatten the |
|
Also if you haven't already you should use your new trait in a proof of concept implementation for your own use case. It'd be a shame if we needed to revise the trait after this was already published. |
…sable PATHEXT caching
Yes, that would be good. I'll look into doing that later. Going to convert this PR to draft until I find the time. |
|
This PR should be good now. I tried it out in deno_task_shell, which has its own in-memory representation of the environment and in one of Deno's crates that needs to compile to wasm32-unknown-unknown. |
Xaeroxe
left a comment
Xaeroxe
left a comment
There was a problem hiding this comment.
This looks pretty good, with one exception which is that we aren't running CI tests for the new --no-default-features configuration. Indeed, the command cargo test --no-default-features doesn't currently pass.
|
Testing without a sys implementation seems hard, but maybe we can do it in terms of an in-memory sys implementation. |
|
Yeah I’ll do it tomorrow. |
| pub bins: Vec<PathBuf>, | ||
| } | ||
| #[cfg(feature = "real-sys")] | ||
| mod real_sys { |
There was a problem hiding this comment.
Note: No changes, just nested in this module in order to conditionally compile this. Scroll down to mod in_memory for new stuff (though the diff is terrible... might be better to just view the file)
| actual.display() | ||
| ); | ||
| } | ||
| fn with_entry_mut(&mut self, path: impl AsRef<Path>) -> Option<&mut DirectoryEntry> { |
There was a problem hiding this comment.
I lazily used AI for this and some other methods, but it seems to be working ok and this is just test code.
| fn tilde_path() { | ||
| let mut sys = InMemorySys::new(); | ||
| sys.set_home_dir("/home/user/"); | ||
| sys.set_env_var("PATH", "/dir/:~/sub/"); |
There was a problem hiding this comment.
I'm actually surprised this crate has this behaviour, but it's nice it can be disabled now by overriding the home_dir implementation to return None.
Xaeroxe
left a comment
Xaeroxe
left a comment
There was a problem hiding this comment.
Overall looks good, with a couple nitpicks. In addition to the comment given, please don't fully qualify types like std::collection::HashSet, I'd prefer to add those to an import within the mod declarations.
Can you point out where these are? I scrolled through the code and didn't see anything that stood out. (not sure this is a good use of time tbh) |
| #[inline] | ||
| pub(crate) fn canonicalize(&self, path: &Path) -> io::Result<PathBuf> { | ||
| #[allow(clippy::disallowed_methods)] // ok, sys implementation | ||
| std::fs::canonicalize(path) |
There was a problem hiding this comment.
Ah, maybe these ones? This is done to prevent having to have #[cfg...] on use statements, which starts to get unwieldy IMO (also makes things annoying when removing/chaning sections of code because then clippy errors only happen depending on how the compilation is)
|
This branch has merge conflicts that need to be resolved |
|
It doesn't seem to? Did you want me to rebase and squash or something?
|
|
I see, okay, my button was set to "rebase and merge" but the other merge options work fine. |
2 participants
Creates a
Systrait and puts any system related functionality behind it.This has several benefits:
RealSysimplementation).env_home,rustix, andwinsafeif they already have code or other dependencies for doing this.which::Sysimplementation.