Add rules #171
Add rules #171
Conversation
cyx
force-pushed
the
add-rules
branch
2 times, most recently
from
fa79f50 to
9f9a487
Compare
internal/cli/flags.go
Outdated
| if !f.IsRequired { | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
There was a problem hiding this comment.
I think this change fixes it.
There was a problem hiding this comment.
So when I undo this change even rebased on main I still get prompted for the Name. I think despite us passing isRequired in TextInput -- we'll still end up prompting for the Name -- which isn't the intent, correct?
There was a problem hiding this comment.
That's correct, if it is not a required input it should not prompt for it on update.
There was a problem hiding this comment.
With this change apps update no longer prompts for callback URL. We should add a property for always prompting for that use case then.
internal/cli/flags.go
Outdated
| // Required clones an existing flag and assigns `IsRequired` to true. This is | ||
| // useful when there are flags with several different use cases -- e.g. create | ||
| // requiring the flag, but update not requiring it. | ||
| func (f *Flag) Required() *Flag { | ||
| clone := *f | ||
| clone.IsRequired = true | ||
| return &clone | ||
| } | ||
|
|
internal/cli/rules.go
Outdated
| if err := ruleName.Required().Ask(cmd, &inputs.Name); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
This is the bit which utilizes this new proposed feature of adding Required() fluent semantics to only have to define a flag once and re-define it based on usecase.
There was a problem hiding this comment.
Should't using AskU on the update command handle that?
There was a problem hiding this comment.
There was a problem hiding this comment.
Gotcha, yeah I think maybe I might have had a different expectation. So to step back, wanna clarify:
Given
Name: required
Create
Should always prompt for Name, and will fail if none is supplied.
Update
Should not prompt for Name, and will not fail if none is supplied.
Is my understanding of intent correct?
There was a problem hiding this comment.
Yup.
On create, if not passed the command should prompt for it.
On update should only prompt if no local flag (any at all) is passed.
If Name wasn't a required input, it should not be prompted on update (regardless of local flags), unless manually specified otherwise (e.g. an important but not required field).
There was a problem hiding this comment.
We should document this somewhere.
There was a problem hiding this comment.
Gotcha, based on the callback URL discussion wonder if we need a different property (e.g. AlwaysPrompt bool) or something similar for the case of callback URL in matias' post.
In this case, if we had AlwaysAsk: false in the Name field for rules, we can then skip asking for it on update (even if there were no local flags passed). What do you think?
There was a problem hiding this comment.
There was a problem hiding this comment.
Gotcha, based on the callback URL discussion wonder if we need a different property (e.g.
AlwaysPrompt bool) or something similar for the case of callback URL in matias' post.In this case, if we had
AlwaysAsk: falsein theNamefield for rules, we can then skip asking for it on update (even if there were no local flags passed). What do you think?
Yes, adding a property for that would solve this use case. In the first PoC for the refactor I added it and then removed because it wasn't needed yet. Now it is 🙂
internal/cli/rules.go
Outdated
| }, | ||
| } | ||
|
|
||
| ruleName.Required().RegisterString(cmd, &inputs.Name, "") |
Pick 5 templates to start.
Description
This adds LIST, SHOW, CREATE, UPDATE, and DELETE operations for rules. At the moment we've left out the
Orderfield altogether -- we can either consider having that in the future or completely leave re-ordering in the UI since that's a lot more intuitive.Demo