feat(exit): suspended jobs warning and sending kill signals #287
Conversation
get's decreased every input and set to 2 every exit so if another exit is called in the same or next input it's not 0.
|
sorry mistake 😅 |
|
Thanks for your contribution, @John-Bush14 ! It's great to see progress on this feature. Please don't hesitate to reach out here or in the Discussions area of the project if you have any questions along the process. I've also enabled the checks to start running, so you may find some new results that will need to be looked at. We'll aim to review the changes in detail over the weekend or early next week. |
There was a problem hiding this comment.
Thanks for putting these changes together! It's quite helpful that you were willing to pick up one of our active issues.
The code looks relatively straightforward; I've left a few questions and comments after a more detailed review. Don't hesitate to ask questions here if you have any.
| @@ -42,6 +42,9 @@ pub struct Shell { | |||
| // Additional state | |||
| /// The status of the last completed command. | |||
| pub last_exit_status: u8, | |||
|
|
|||
| /// User tried exiting in the previous/current input if != 0. | |||
| pub user_tried_exiting: u8, | |||
There was a problem hiding this comment.
Is there a reason for this to be an integer instead of a simple bool flag? Are there cases in which you think we'd need to track how many times the user tried exiting?
| @@ -408,9 +408,9 @@ impl Job { | |||
| } | |||
|
|
|||
| /// Kills the job. | |||
| pub fn kill(&mut self) -> Result<(), error::Error> { | |||
| pub fn kill(&mut self, signal: Option<nix::sys::signal::Signal>) -> Result<(), error::Error> { | |||
There was a problem hiding this comment.
You may run into challenges exposing a nix type in this function--I recall it being used in non-Unix platforms (e.g., Windows). An alternative approach might be to define a separate abstract enum type for signals we can send to jobs. It might require adding some new functions under the sys module too.
| @@ -14,6 +16,18 @@ impl builtins::Command for ExitCommand { | |||
| &self, | |||
| context: commands::ExecutionContext<'_>, | |||
| ) -> Result<crate::builtins::ExitCode, crate::error::Error> { | |||
| if !context.shell.jobs.jobs.is_empty() && context.shell.user_tried_exiting == 0 { | |||
| writeln!(context.stdout(), "brush: You have suspended jobs.")?; | |||
There was a problem hiding this comment.
Minor code note: would be nice to avoid hard-coding the display name of the shell. If I recall correctly Shell::shell_name can be used.
| @@ -1,5 +1,7 @@ | |||
| use clap::Parser; | |||
|
|
|||
| use std::io::Write; | |||
There was a problem hiding this comment.
Right now, a Ctrl+D based exit won't actually invoke the exit built-in. Do we need to make an additional change to make sure that path also works in the same way?
reubeno
added
updates_requested
exitwarns user if user has suspended jobs running and sends SIGHUP's on second try.