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

Accept SIGINT, INT, and int along with sigint as a vallid signal name. #455

Closed
GrigorenkoPV opened this issue Aug 16, 2023 · 4 comments · Fixed by #456
Closed

Accept SIGINT, INT, and int along with sigint as a vallid signal name. #455

GrigorenkoPV opened this issue Aug 16, 2023 · 4 comments · Fixed by #456
Labels
s: Client This issue touches the pueue client t: Enhancement Something that exists, but could be made better

Comments

@GrigorenkoPV
Copy link

A detailed description of the feature you would like to see added.

For the -s/--signal parameter of the pueue kill command, it would be cool to be able to use signals' names both in lowercase & uppercase as well as both with the "SIG" prefix & without it.

Explain your usecase of the requested feature

An actual snippet from my terminal just now.
I got "lucky" to correctly guess the format only on the 4th attempt.pueu

$ pueue kill -s INT 0
error: invalid value 'INT' for '--signal <SIGNAL>': Matching variant not found

For more information, try '--help'.
2 $ pueue kill -s SIGINT 0
error: invalid value 'SIGINT' for '--signal <SIGNAL>': Matching variant not found

For more information, try '--help'.
2 $ pueue kill -s int 0
error: invalid value 'int' for '--signal <SIGNAL>': Matching variant not found

For more information, try '--help'.
2 $ pueue kill -s sigint 0
Tasks are being killed: 0
$

At that point I was starting to get worried that I would have to pass the numerical value for the signal, which I don't remember for SIGINT. 😅

Alternatives

No response

Additional context

No response

@Nukesor Nukesor added t: Enhancement Something that exists, but could be made better s: Client This issue touches the pueue client labels Aug 18, 2023
@Nukesor
Copy link
Owner

Nukesor commented Aug 18, 2023

Could you check if the linked PR covers all variations :)?

@GrigorenkoPV
Copy link
Author

Could you check if the linked PR covers all variations :)?

Well, potentially "SigInt" is not covered, but that's kind of ridiculous. However, makes me wonder if there's a way to make the comparison case-insensitive.

I've taken a look at strum docs, there's an ascii_case_insensitive thing, which looks like it can help to get rid of

  serialize = "SigKill",
  serialize = "sigkill",
  serialize = "SIGKILL",

boilerplate.

And this seems to suggest that ascii_case_insensitive can be applied to the entire enum at once?

@GrigorenkoPV
Copy link
Author

diff --git a/pueue_lib/src/network/message.rs b/pueue_lib/src/network/message.rs
index e65c826..2d3c157 100644
--- a/pueue_lib/src/network/message.rs
+++ b/pueue_lib/src/network/message.rs
@@ -187,16 +187,17 @@ impl_into_message!(PauseMessage, Message::Pause);
 /// This is also needed for usage in clap, since nix's Signal doesn't implement [Display] and
 /// [std::str::FromStr].
 #[derive(PartialEq, Eq, Clone, Debug, Deserialize, Serialize, Display, EnumString)]
+#[strum(ascii_case_insensitive)]
 pub enum Signal {
-    #[strum(serialize = "SigInt", serialize = "sigint", serialize = "2")]
+    #[strum(serialize = "int", serialize = "sigint", serialize = "2")]
     SigInt,
-    #[strum(serialize = "SigKill", serialize = "sigkill", serialize = "9")]
+    #[strum(serialize = "kill", serialize = "sigkill", serialize = "9")]
     SigKill,
-    #[strum(serialize = "SigTerm", serialize = "sigterm", serialize = "15")]
+    #[strum(serialize = "term", serialize = "sigterm", serialize = "15")]
     SigTerm,
-    #[strum(serialize = "SigCont", serialize = "sigcont", serialize = "18")]
+    #[strum(serialize = "cont", serialize = "sigcont", serialize = "18")]
     SigCont,
-    #[strum(serialize = "SigStop", serialize = "sigstop", serialize = "19")]
+    #[strum(serialize = "stop", serialize = "sigstop", serialize = "19")]
     SigStop,
 }

Something like this

@Nukesor
Copy link
Owner

Nukesor commented Aug 21, 2023

Nice, good catch.

I somehow expected that the case_insensitive option didn't work in combination with custom serialize attributes, but I turned out to be wrong :))

@Nukesor Nukesor closed this as completed Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: Client This issue touches the pueue client t: Enhancement Something that exists, but could be made better
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants