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

Allows multiple commands to watch #24

Merged
merged 6 commits into from
Oct 17, 2023
Merged

Conversation

yozhgoor
Copy link
Member

@yozhgoor yozhgoor commented Mar 2, 2023

No description provided.

@yozhgoor yozhgoor requested a review from cecton April 11, 2023 14:55
src/lib.rs Outdated
Comment on lines 470 to 471
let _ = child.wait();
let _ = child.kill();
Copy link
Member

Choose a reason for hiding this comment

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

haha 😁 you actually need to do the opposite. Otherwise you will wait for the child to complete

src/lib.rs Outdated
@@ -440,6 +427,52 @@ impl EventHandler for WatchEventHandler {
}
}

struct ShareableChild(Arc<Mutex<Option<Child>>>);
Copy link
Member

Choose a reason for hiding this comment

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

I think I would rather call it SharedChild...... not sure why tbh....

src/lib.rs Outdated
@@ -107,7 +107,7 @@
//! match opt {
//! Opt::Watch(watch) => {
//! log::info!("Starting to watch `cargo check`");
//! watch.run(run_command)?;
//! watch.run(vec![run_command])?;
Copy link
Member

Choose a reason for hiding this comment

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

Excellent work!

You see how abstracting the logic behind ShareableChild made everything easier.

Now I would like you to improve the API of the fn run() on this watch object. You see, in the past we could provide a command and it would work. Ideally we want to preserve this behavior AND allow passing an iterable over commands. insert why-not-both.gif here

In order to do that, instead of taking the parameter mut commands: Vec<Command> you can create a new type and take as parameter mut commands: impl Into<CommandList>.

I think I have already said too much so I will leave the rest to you. The goal is to allow this:

  • watch.run(vec![run_command])?; (list of commands, the easiest)
  • watch.run(run_command)?; (single command, easy)
  • watch.run(&[run_command])?; (any iterable actually, you need to get your hand on generic parameters, more complicated, I believe in you)

@cecton cecton merged commit bfce374 into rustminded:main Oct 17, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants