Conversation
| String { | ||
| required: bool, | ||
| default: String, | ||
| short: Option<String>, |
There was a problem hiding this comment.
Opening this PR as a draft because the current implementation doesn't validate that the short option is a single character.
I originally tried to add short with type Option<char> to both the starlark module and TarkArgs but ran into a few problems with unimplemented traits.
- Starlark doesn't have a char type, so adding an argument of type char requires implementing UnpackValue and other traits. I wasn't sure if this is worth doing.
- Using char in TaskArgs presented errors because Allocative is not implemented for char.
Let me know if you'd like to me to further pursue either of those changes.
I can also add a validation in each of the starlark module functions which returns an error if the length of the string != 1. That's probably the simplest way forward, but I wanted to see if there's a better way to do this in the type system.
There was a problem hiding this comment.
String is the way to go, starlark does not have a char type and starlark-rust would have to cast to string anyway. lets make sure we validate length of the string.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea01c549cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| fn short_option(short: &Option<String>) -> Resettable<char> { | ||
| match short { | ||
| Some(text) => Resettable::Value(text.chars().nth(0).unwrap().into()), |
There was a problem hiding this comment.
The new short-flag path can panic on user input: short is accepted as an arbitrary String in the task arg constructors, but this helper unconditionally does text.chars().nth(0).unwrap(). If a task defines short = "" for any named arg, command construction crashes instead of returning a configuration error, so the CLI exits before running the task. Please guard empty strings (and ideally enforce exactly one-character shorts) and surface a normal validation error.
Useful? React with 👍 / 👎.
2 participants
This PR adds short options for all TaskArgs (except Positional and TrailingVarArgs).
Closes #925.
Changes are visible to end-users: yes
Task args can now include a short option in addition.
Test plan