-
Notifications
You must be signed in to change notification settings - Fork 11
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
CLI flag for disabling interrupts #186
Conversation
…nologies/ghciwatch into mattp/cli-flag-for-interrupt
src/cli.rs
Outdated
/// By default, ghciwatch will interrupt reloads if a file changes. If you want ghciwatch to | ||
/// avoid interrupting reloads, set this flag. | ||
#[arg(long = "no-interrupt-reloads")] | ||
pub no_interrupt_reloads: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super happy with a --no-interrupt-reloads
flag - feels kind of awkward. But --interrupt-reloads
is the default behavior.
We could switch the default back to no-interrupt
, and then this flag can be --interrupt-reloads
.
Made a slack convo to ask about this:
https://mercurytechnologies.slack.com/archives/CUJ89R895/p1702483630925389
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an open issue for automatic negation flags (i.e. adding --interrupt-reloads
and --no-interrupt-reloads
at once) here:
@@ -56,6 +56,7 @@ pub async fn run_ghci( | |||
// This function is pretty tricky! We need to handle shutdowns at each stage, and the process | |||
// is a little different each time, so the `select!`s can't be consolidated. | |||
|
|||
let no_interrupt_reloads = opts.no_interrupt_reloads; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely feels awkward. I tried passing the opts
directly in to the relevant function, but the opts
are moved into Ghci::new
call just below. I can get the bool
out here no problem, though, so I do that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think this is fine.
src/ghci/manager.rs
Outdated
// Send a SIGINT to interrupt the reload. | ||
// NB: This may take a couple seconds to register. | ||
ghci.lock().await.send_sigint().await?; | ||
if !no_interrupt_reloads { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here's our behavior change: if we set this flag, then we do not go into that block
This PR adds a cli flag that disables the interrupt behavior for reloads. This is my first contribution to a Rust project so I'm sure it'll take some polishing :)