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.

Sign up for GitHub

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

Simplify the API and inner implementation #85

Merged
merged 2 commits into from
Dec 22, 2024
+221 −291
Merged

Simplify the API and inner implementation #85

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a31894
Refactor the API to use Arc<OSstr> and remove refcells
elichai Apr 14, 2024
1f775f4
Apply review comments
elichai May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ fn main() -> anyhow::Result<()> {
let user = "matklad";
let repo = "xshell";
cmd!(sh, "git clone https://github.com/{user}/{repo}.git").run()?;
sh.change_dir(repo);
sh.set_current_dir(repo);

let test_args = ["-Zunstable-options", "--report-time"];
cmd!(sh, "cargo test -- {test_args...}").run()?;
Expand All @@ -29,7 +29,7 @@ fn main() -> anyhow::Result<()> {

cmd!(sh, "git tag {version}").run()?;

let dry_run = if sh.var("CI").is_ok() { None } else { Some("--dry-run") };
let dry_run = if sh.env_var("CI").is_ok() { None } else { Some("--dry-run") };
cmd!(sh, "cargo publish {dry_run...}").run()?;

Ok(())
Expand Down
2 changes: 1 addition & 1 deletion examples/ci.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ fn publish(sh: &Shell) -> Result<()> {

if current_branch == "master" && !tag_exists {
// Could also just use `CARGO_REGISTRY_TOKEN` environmental variable.
let token = sh.var("CRATES_IO_TOKEN").unwrap_or("DUMMY_TOKEN".to_string());
let token = sh.env_var("CRATES_IO_TOKEN").unwrap_or("DUMMY_TOKEN".to_string());
cmd!(sh, "git tag v{version}").run()?;
cmd!(sh, "cargo publish --token {token} --package xshell-macros").run()?;
cmd!(sh, "cargo publish --token {token} --package xshell").run()?;
Expand Down
4 changes: 2 additions & 2 deletions examples/clone_and_publish.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ fn main() -> anyhow::Result<()> {
let user = "matklad";
let repo = "xshell";
cmd!(sh, "git clone https://github.com/{user}/{repo}.git").run()?;
sh.change_dir(repo);
sh.set_current_dir(repo);

let test_args = ["-Zunstable-options", "--report-time"];
cmd!(sh, "cargo test -- {test_args...}").run()?;
Expand All @@ -21,7 +21,7 @@ fn main() -> anyhow::Result<()> {

cmd!(sh, "git tag {version}").run()?;

let dry_run = if sh.var("CI").is_ok() { None } else { Some("--dry-run") };
let dry_run = if sh.env_var("CI").is_ok() { None } else { Some("--dry-run") };
cmd!(sh, "cargo publish {dry_run...}").run()?;

Ok(())
Expand Down
98 changes: 60 additions & 38 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@
//! ```no_run
//! # use xshell::{Shell, cmd}; let mut sh = Shell::new().unwrap();
//! # let repo = "xshell";
//! sh.change_dir(repo);
//! sh.set_current_dir(repo);
//! ```
//!
//! Each instance of [`Shell`] has a current directory, which is independent of
Expand Down Expand Up @@ -180,7 +180,7 @@
//!
//! ```no_run
//! # use xshell::{Shell, cmd}; let sh = Shell::new().unwrap();
//! let dry_run = if sh.var("CI").is_ok() { None } else { Some("--dry-run") };
//! let dry_run = if sh.env_var("CI").is_ok() { None } else { Some("--dry-run") };
//! cmd!(sh, "cargo publish {dry_run...}").run()?;
//! # Ok::<(), xshell::Error>(())
//! ```
Expand All @@ -196,7 +196,7 @@
//! let user = "matklad";
//! let repo = "xshell";
//! cmd!(sh, "git clone https://github.com/{user}/{repo}.git").run()?;
//! sh.change_dir(repo);
//! sh.set_current_dir(repo);
//!
//! let test_args = ["-Zunstable-options", "--report-time"];
//! cmd!(sh, "cargo test -- {test_args...}").run()?;
Expand All @@ -210,7 +210,7 @@
//!
//! cmd!(sh, "git tag {version}").run()?;
//!
//! let dry_run = if sh.var("CI").is_ok() { None } else { Some("--dry-run") };
//! let dry_run = if sh.env_var("CI").is_ok() { None } else { Some("--dry-run") };
//! cmd!(sh, "cargo publish {dry_run...}").run()?;
//!
//! Ok(())
Expand Down Expand Up @@ -374,7 +374,7 @@ macro_rules! cmd {
/// use xshell::{cmd, Shell};
///
/// let sh = Shell::new()?;
/// let sh = sh.push_dir("./target");
/// let sh = sh.with_current_dir("./target");
/// let cwd = sh.current_dir();
/// cmd!(sh, "echo current dir is {cwd}").run()?;
///
Expand All @@ -385,7 +385,7 @@ macro_rules! cmd {
#[derive(Debug, Clone)]
pub struct Shell {
cwd: Arc<Path>,
env: HashMap<Arc<OsStr>, Arc<OsStr>>,
env: Arc<HashMap<Arc<OsStr>, Arc<OsStr>>>,
}

impl std::panic::UnwindSafe for Shell {}
Expand All @@ -397,7 +397,7 @@ impl Shell {
/// Fails if [`std::env::current_dir`] returns an error.
pub fn new() -> Result<Shell> {
let cwd = current_dir().map_err(|err| Error::new_current_dir(err, None))?;
Ok(Shell { cwd: cwd.into(), env: HashMap::new() })
Ok(Shell { cwd: cwd.into(), env: Default::default() })
}

// region:env
Expand All @@ -413,11 +413,11 @@ impl Shell {
/// Changes the working directory for this [`Shell`].
///
/// Note that this doesn't affect [`std::env::current_dir`].
#[doc(alias = "pwd")]
pub fn change_dir(&mut self, dir: impl AsRef<Path>) {
self._change_dir(dir.as_ref().as_ref())
#[doc(alias = "cd")]
pub fn set_current_dir(&mut self, dir: impl AsRef<Path>) {
self._set_current_dir(dir.as_ref().as_ref())
}
fn _change_dir(&mut self, dir: &OsStr) {
fn _set_current_dir(&mut self, dir: &OsStr) {
self.cwd = self.cwd.join(dir).into();
}

Expand All @@ -426,10 +426,10 @@ impl Shell {
/// Note that this doesn't affect [`std::env::current_dir`].
#[doc(alias = "pushd")]
#[must_use]
pub fn push_dir(&self, path: impl AsRef<Path>) -> Self {
self._push_dir(path.as_ref())
pub fn with_current_dir(&self, path: impl AsRef<Path>) -> Self {
self._with_current_dir(path.as_ref())
}
fn _push_dir(&self, path: &Path) -> Self {
fn _with_current_dir(&self, path: &Path) -> Self {
Self { cwd: self.cwd.join(path).into(), env: self.env.clone() }
}

Expand All @@ -439,11 +439,11 @@ impl Shell {
///
/// Environment of the [`Shell`] affects all commands spawned via this
/// shell.
pub fn var(&self, key: impl AsRef<OsStr>) -> Result<String> {
self._var(key.as_ref())
pub fn env_var(&self, key: impl AsRef<OsStr>) -> Result<String> {
self._env_var(key.as_ref())
}
fn _var(&self, key: &OsStr) -> Result<String> {
match self._var_os(key) {
fn _env_var(&self, key: &OsStr) -> Result<String> {
match self._env_var_os(key) {
Some(it) => match it.to_str() {
Some(it) => Ok(it.to_string()),
None => Err(VarError::NotUnicode(key.into())),
Expand All @@ -458,22 +458,45 @@ impl Shell {
///
/// Environment of the [`Shell`] affects all commands spawned via this
/// shell.
pub fn var_os(&self, key: impl AsRef<OsStr>) -> Option<Arc<OsStr>> {
self._var_os(key.as_ref())
pub fn env_var_os(&self, key: impl AsRef<OsStr>) -> Option<Arc<OsStr>> {
self._env_var_os(key.as_ref())
}
fn _var_os(&self, key: &OsStr) -> Option<Arc<OsStr>> {
fn _env_var_os(&self, key: &OsStr) -> Option<Arc<OsStr>> {
self.env.get(key).cloned().or_else(|| env::var_os(key).map(Into::into))
}

/// Fetches the whole environment as a `(Key, Value)` iterator for this [`Shell`].
///
/// Returns an error if any of the variables are not utf8.
///
/// Environment of the [`Shell`] affects all commands spawned via this
/// shell.
pub fn env_vars(&self) -> Result<impl Iterator<Item = (&str, &str)>> {
if let Some((key, _)) =
self.env_vars_os().find(|(a, b)| a.to_str().or(b.to_str()).is_none())
{
return Err(Error::new_var(VarError::NotUnicode(key.into()), key.into()));
}
Ok(self.env_vars_os().map(|(k, v)| (k.to_str().unwrap(), v.to_str().unwrap())))
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this looks weird. I'd expect either Resut<Vec<_>> or impl Iterator (with a panic on next).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Panic on next? Did you mean Result on next?
If so, this is not super ergonomic unless you call collect on it (hard to chain maps/filters and another variable in a loop)

Should we just return Result<HashMap<... >>? Like, converting to a Vec of pairs also feels a little weird, but I'm really not sure what's the "right" option here

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant panic on next, like the std does.

the API which I think makes sense is returning Result Vec. The api that matches std is a panicky iterator. To be honest, I am a bit confused how to chose between the two.

But returning an iterator while also doing proactive utf8 validation seems inferior to either


/// Fetches the whole environment as a `(Key, Value)` iterator for this [`Shell`].
///
/// Environment of the [`Shell`] affects all commands spawned via this
/// shell.
pub fn env_vars_os(&self) -> impl Iterator<Item = (&OsStr, &OsStr)> {
self.env.iter().map(|(k, v)| (k.as_ref(), v.as_ref()))
}

/// Sets the value of `key` environment variable for this [`Shell`] to
/// `val`.
///
/// Note that this doesn't affect [`std::env::var`].
pub fn set_var(&mut self, key: impl AsRef<OsStr>, val: impl AsRef<OsStr>) {
self._set_var(key.as_ref(), val.as_ref())
pub fn set_env_var(&mut self, key: impl AsRef<OsStr>, val: impl AsRef<OsStr>) {
self._set_env_var(key.as_ref(), val.as_ref())
}
fn _set_var(&mut self, key: &OsStr, val: &OsStr) {
self.env.insert(key.into(), val.into());
fn _set_env_var(&mut self, key: &OsStr, val: &OsStr) {
Arc::make_mut(&mut self.env).insert(key.into(), val.into());
}

// endregion:env
Expand Down Expand Up @@ -670,10 +693,9 @@ impl Shell {
#[derive(Debug, Clone)]
#[must_use]
pub struct Cmd {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, we are getting rid of lifetime on Cmd? But this is excellent! This makes me think that we do want to go this way, feels so much simpler now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yess :D My main motivation is to remove the lifetime and the RefCells that complicated passing refrences due to borrow handles

cwd: Arc<Path>,
sh: Shell,
prog: PathBuf,
args: Vec<OsString>,
envs: HashMap<Arc<OsStr>, Arc<OsStr>>,
ignore_status: bool,
quiet: bool,
secret: bool,
Expand Down Expand Up @@ -709,11 +731,10 @@ impl From<Cmd> for Command {
}

impl Cmd {
fn new(shell: &Shell, prog: impl AsRef<Path>) -> Self {
fn new(sh: &Shell, prog: impl AsRef<Path>) -> Self {
Cmd {
cwd: shell.cwd.clone(),
sh: sh.clone(),
prog: prog.as_ref().into(),
envs: shell.env.clone(),
args: Vec::new(),
ignore_status: false,
quiet: false,
Expand Down Expand Up @@ -767,7 +788,7 @@ impl Cmd {
}

fn _env_set(&mut self, key: &OsStr, val: &OsStr) {
self.envs.insert(key.into(), val.into());
Arc::make_mut(&mut self.sh.env).insert(key.into(), val.into());
}

/// Overrides the values of specified environmental variables for this
Expand All @@ -778,7 +799,8 @@ impl Cmd {
K: AsRef<OsStr>,
V: AsRef<OsStr>,
{
self.envs.extend(vars.into_iter().map(|(k, v)| (k.as_ref().into(), v.as_ref().into())));
Arc::make_mut(&mut self.sh.env)
.extend(vars.into_iter().map(|(k, v)| (k.as_ref().into(), v.as_ref().into())));
self
}

Expand All @@ -788,12 +810,12 @@ impl Cmd {
self
}
fn _env_remove(&mut self, key: &OsStr) {
self.envs.remove(key);
Arc::make_mut(&mut self.sh.env).remove(key);
}

/// Removes all of the environment variables from this command.
pub fn env_clear(mut self) -> Self {
self.envs.clear();
Arc::make_mut(&mut self.sh.env).clear();
self
}

Expand Down Expand Up @@ -938,8 +960,8 @@ impl Cmd {
// directory does not exist. Return an appropriate error in such a
// case.
if matches!(err.kind(), io::ErrorKind::NotFound) {
if let Err(err) = self.cwd.metadata() {
return Error::new_current_dir(err, Some(self.cwd.clone()));
if let Err(err) = self.sh.cwd.metadata() {
return Error::new_current_dir(err, Some(self.sh.cwd.clone()));
}
}
Error::new_cmd_io(self, err)
Expand All @@ -966,10 +988,10 @@ impl Cmd {

fn to_command(&self) -> Command {
let mut res = Command::new(&self.prog);
res.current_dir(&self.cwd);
res.current_dir(&self.sh.cwd);
res.args(&self.args);

for (key, val) in &self.envs {
for (key, val) in &*self.sh.env {
res.env(key, val);
}

Expand Down
4 changes: 2 additions & 2 deletions tests/compile_time.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@ use xshell::{cmd, Shell};
fn fixed_cost_compile_times() {
let mut sh = Shell::new().unwrap();

let _p = sh.change_dir("tests/data");
let _p = sh.set_current_dir("tests/data");
let baseline = compile_bench(&mut sh, "baseline");
let _ducted = compile_bench(&sh, "ducted");
let xshelled = compile_bench(&mut sh, "xshelled");
let ratio = (xshelled.as_millis() as f64) / (baseline.as_millis() as f64);
assert!(1.0 < ratio && ratio < 10.0);

fn compile_bench(sh: &Shell, name: &str) -> Duration {
let sh = sh.push_dir(name);
let sh = sh.with_current_dir(name);
let cargo_build = cmd!(sh, "cargo build -q");
cargo_build.read().unwrap();

Expand Down
2 changes: 1 addition & 1 deletion tests/it/compile_failures.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ fn check(code: &str, err_msg: &str) {
let mut sh = Shell::new().unwrap();
let xshell_dir = sh.current_dir().to_owned();
let temp_dir = sh.create_temp_dir().unwrap();
sh.change_dir(temp_dir.path());
sh.set_current_dir(temp_dir.path());

let manifest = format!(
r#"
Expand Down
8 changes: 4 additions & 4 deletions tests/it/env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ fn test_env() {
);
}

let _g1 = sh.set_var(v1, "foobar");
let _g2 = sh.set_var(v2, "quark");
let _g1 = sh.set_env_var(v1, "foobar");
let _g2 = sh.set_env_var(v2, "quark");

assert_env(cmd!(sh, "xecho -$ {v1} {v2}"), &[(v1, Some("foobar")), (v2, Some("quark"))]);
assert_env(cmd!(cloned_sh, "xecho -$ {v1} {v2}"), &[(v1, None), (v2, None)]);
Expand Down Expand Up @@ -79,8 +79,8 @@ fn test_env_clear() {
&[(v1, Some("789")), (v2, None)],
);

let _g1 = sh.set_var(v1, "foobar");
let _g2 = sh.set_var(v2, "quark");
let _g1 = sh.set_env_var(v1, "foobar");
let _g2 = sh.set_env_var(v2, "quark");

assert_env(cmd!(sh, "{xecho} -$ {v1} {v2}").env_clear(), &[(v1, None), (v2, None)]);
assert_env(
Expand Down
Loading
Loading