clap icon indicating copy to clipboard operation
clap copied to clipboard

Add support for ignoring errors on subcommands

Open cd-work opened this issue 2 years ago • 7 comments

Currently when setting ignore_errors(true) anywhere but the top level, it would not have any effect. As a result this means it's not possible to ignore errors for a subcommand while still enforcing correctness on the top level.

This patch makes it possible to set ignore_errors(true) anywhere in the nested subcommand chain and all children will ignore errors without affecting the parent elements.

This is particularly helpful for external subcommands that are still documented on the top level, making it possible to generically forward all parameters while still documenting the externals subcommand on the top level.

cd-work avatar Jun 29 '22 12:06 cd-work

I'd assume commit message can be easily transformed to read whatever you want when merging.

cd-work avatar Jun 29 '22 13:06 cd-work

I'd assume commit message can be easily transformed to read whatever you want when merging.

Yup, the error message is prefaced with a comment saying just that

If this fails, don't sweat it. We're trying to encourage clear communication and not hinder contributions. If it is a reasonable issue and you lack time or feel uncomfortable fixing it yourself, let us know and we can mentor or fix it.

epage avatar Jun 30 '22 01:06 epage

This is particularly helpful for external subcommands that are still documented on the top level, making it possible to generically forward all parameters while still documenting the externals subcommand on the top level.

So if I'm understanding your use case, you are creating a Command that is really just forwarding to an external subcommand, instead of using the allow_external_subcommand feature?

epage avatar Jun 30 '22 01:06 epage

Could you open an issue first? I prefer Problem and Solution discussions to happen in issues, leaving PRs for Implementation discussions. If nothing else, I commonly see multiple PRs exist for one Problem and leaving the discussion in the PR can fragment the conversation.

Things we'll need to explore

  • A concise description of the current behavior
  • A concise description of the proposed behavior
  • The proposed behavior's impact on compatibility

When talking about the behavior, we need to keep in mind that Command::ignore_errors is set recursively through the subcommand tree (ie global)

epage avatar Jun 30 '22 01:06 epage

Still catching up on a backlog of messages and saw you had #3863. I went ahead and turned into issue #3887.

epage avatar Jun 30 '22 02:06 epage

I agree that the underlying problem could benefit from a bit of discussion, but I feel like either way this patch would be useful just as a fix to the existing ignore_errors behavior. I personally got pretty confused when I realized that it only works when you set it on the topmost option, but doesn't when you set it on a child.

cd-work avatar Jun 30 '22 11:06 cd-work

I agree that the underlying problem could benefit from a bit of discussion, but I feel like either way this patch would be useful just as a fix to the existing ignore_errors behavior. I personally got pretty confused when I realized that it only works when you set it on the topmost option, but doesn't when you set it on a child.

I feel the solution needs discussion in addition to the problem. Note that the talking points I suggest include dealing with the solution. I'd like to make sure we are nailing down the right semantics in a compatible way.

epage avatar Jun 30 '22 13:06 epage

At this point, it looks like this needs an issue for having a full discussion on this. I'm closing this until that has happened.

epage avatar Jul 21 '23 15:07 epage