Add command-level and task-level warning

[parkerduckworth]

Hey @andreynering, just saw your comment on issue #100 regarding the pending upstream ability to pass a -p flag to read. Coincidentally, I just finished writing this patch before your reply.

I understand that mdvan/sh#551 will clear up the ability to add a prompt, but I think that having an explicit warning option at both task and command levels may improve this tool. What do you think?

Here is how warning can be used at both levels:

version: '3'

tasks:
  example-task:
    desc: An example
    warning: Are you sure you want to run this task?
    cmds:
      - cmd: rm -rf /important-files
        warning: Are you sure you want to run this command?
      - rm -rf /unimportant-files

Declining to run a command with a command-level warning results in the command being skipped, and the rest of the commands in the task (without warnings) will run unhindered.

I know you will have this project's best interest in mind. Thanks for considering this feature!

[andreynering]

Hi @parkerduckworth, thanks for the PR! 🙂

The code seems simple enough to be worth adding this. I'll probably check this in another day, though...

In the meantime, can you change the base branch from master to v3 and rebase?

[parkerduckworth]

Absolutely!

[parkerduckworth]

Ok @andreynering , everything looks to be in order!

[andreynering]

Hi @parkerduckworth,

Sorry for the wait, been busy recently...

Some comments so far:

• I think that prompt may be a better name than warning. What do you think?
• I think it's a bit inconsistent that command prompts skips the command while task prompts makes it error. I think both should have the same behavior, and I believe the right behavior would be to skip the task. Sounds good?
• Many CLI tools have a -y argument to bypass prompts and execute then without asking. This is important to allow executing these commands on CI, for example.
• [Optional] A small section on the documentation about this would be nice. But if you don't want to do it, I'll after the merge

[parkerduckworth]

@andreynering no worries, life is to be lived!

---

I think that prompt may be a better name than warning. What do you think?

I think that prompt offers more flexibility! At the moment, these changes append a yes/no option at the end of the supplied warning, i.e. fmt.Printf("%s (y/N): ", warning). If we go with prompt, this should probably change to print only the provided prompt (fmt.Printf("%s: ", prompt), as a prompt may often require more than a yes or no answer.

So maybe an example would be like this:

tasks:
  example-task:
    desc: An example
    prompt: Are you sure you want to run this task? (y/N)

I think the answer depends on whether or not we want more flexibility here. You have obviously spent much more time than I working with this codebase, so I would happily agree with your preference.

---

I think it's a bit inconsistent that command prompts skips the command while task prompts makes it error. I think both should have the same behavior, and I believe the right behavior would be to skip the task. Sounds good?

Excellent! So shall it be.

---

Many CLI tools have a -y argument to bypass prompts and execute then without asking. This is important to allow executing these commands on CI, for example.

I agree! We should provide this affordance as well.

---

[Optional] A small section on the documentation about this would be nice. But if you don't want to do it, I'll after the merge

Ha! I would be more than happy to write up the docs for this!!

---

If you are good with all my above remarks, I will make these changes and push them up.

[andreynering]

Thinking better, let's keep warning for a matter of clarity since it'll only have two options (yes and no, not sure how we'd support more)

On the other points, I think we agree about them, so feel free to proceed =)

[parkerduckworth]

Requested changes have been made 👍

[parkerduckworth]

Hey @andreynering, you mind sharing why this was closed?

[andreynering]

@parkerduckworth Sorry, this was a mistake

For some reason GitHub closed many PRs after merging #220

Reopening...

[parkerduckworth]

Hello @andreynering ! Is this feature still going to be merged?

[parkerduckworth]

Hey @andreynering , this is actually no longer necessary, as I was able to get read -p flag support merged into https://github.com/mvdan/sh with this PR.

Any chance we can cut a new release with the updated mvdan/sh dependency to allow us to pass in -p to read?

Thank you!

[andreynering]

@parkerduckworth Sorry for not responding to you all this time. Life happens.

Good to hear that the read -p works for you. Thanks for your contribution on mvdan/sh#722 as well.

[parkerduckworth]

@andreynering apology not necessary 🙂 Hope all is well mate!