-
Notifications
You must be signed in to change notification settings - Fork 10
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
Proposal for "run" handler at Api level #65
Comments
Addressing your concerns
That hasn't stopped me before 🤣 But, seriously, I don't think this is a problem. It will make sense in certain scenarios, and we can describe it as a way to encapsulate your program's logic once argument parsing is complete. Plus, assuming the top-level
I don't think they really conflict - you have the same issue today if you have logic that uses
Just like you should expect both the command handler AND any logic that comes after My concernsThe one question I have is what arguments should be passed to the top-level run handler. If we want to maintain consistency with a command run handler, then it should be
Because of these issues, I think we should consider one or more of the following:
Personally I think we should do number 1 and not number 2, since number 2 could be confusing. With this approach, we should probably just pass the The end result would look something like this: #!/usr/bin/env node
const cli = require('sywac')
/* ... options, commands, etc ... */
.afterParse(result => {
const myLib = require('./index')
return myLib(result.argv)
})
module.exports = cli
if (require.main === module) cli.parseAndExit() which would allow you to write tests that What do you think? |
Your reasoning seems sound, but I am worried about point (1). In a Command run handler, you are technically mid-parse, which means you have the flexibility to throw an Error or call This flexibility is part of the point of the "top level run handler", in my eyes - more flexible than I guess I would want the run handler to take |
Oh ok, interesting. I wasn't thinking of it that way, but that makes sense. In that case, I assume we would just call it What do you think about my argument to execute both a command handler and the top-level run handler when both are defined/given? Are you ok with that? |
I think I am. It means the command's run handler and your setup run handler behave slightly differently, but that is something we can document, for sure. To get more detailed, here's a test case that I think covers all the bases: // pizza.js
require('sywac')
.check((argv, context) => console.log('[check] level 1'))
.run((argv, context) => console.log('[run] level 1'))
.command('order', {
setup: sywac => {
sywac
.check((argv, context) => console.log('[check] level 2'))
.run((argv, context) => console.log('[run] level 2'))
.command('cancel', {
setup: sywac => {
sywac
.check((argv, context) => console.log('[check] level 3'))
.run((argv, context) => console.log('[run] level 3'))
},
run: (argv, context) => console.log('[run] order cancel')
})
},
run: (argv, context) => console.log('[run] order')
})
.parseAndExit() $ pizza order cancel
[check] level 1
[check] level 2
[check] level 3
[run] order cancel
[run] level 3
[run] level 2
[run] level 1
$ pizza order
[check] level 1
[check] level 2
[run] order
[run] level 2
[run] level 1 Although it's not necessarily intuitive for a first-time user of Sywac that nested run handlers execute in LIFO order, I think this is the way it has to be -- I haven't even looked at how messy it would be to implement, but my take on the requirements is:
|
Next-day thoughts: I might have given up too soon on this order: $ pizza order cancel
[check] level 1
[run] level 1
[check] level 2
[run] level 2
[check] level 3
[run] level 3
[run] order cancel
So anyway, I thought I'd walk back my "it HAS to be this way" statement. But I still think the last-in-first-out |
Thanks for that exercise. I'm not sure your 2nd option (the next-day thought) is viable, because I think we have to (or at least should) run the command handler before running the Api run handler. I don't think it makes sense to try to run the Api handler between the
I think this is a good point that gives weight to the argument for executing the Api run handler last (after I'm leaning toward a third option: explicitly disable support for Assuming sywac is used as intended, we can tell when an Api instance is a child or not. For children, we can simply ignore (or error) when IMO, when a CLI program (or chatbot parser) is run, the goal should be to determine which individual/single handler to execute (regardless of "level") and then execute it with the given/parsed arguments. The concept of "kicking off" more than one handler per command/message seems unintended and possibly inappropriate to me. The only reason child Api instances exist at all is to support commands. The fact that sywac uses a child Api instance to organize arguments and options specific to each command is just an implementation detail - the side effects of that implementation detail should not necessarily be construed as an intended feature. So, the way I'm thinking about it, the Api I should mention that whatever you're trying to achieve in your CLI with a nested Api For these reasons, I don't think we need to support nested levels of Api What do you think? Is there a valid use-case for wanting to run multiple |
Nope! I like your suggestion, as I think we can state it really plainly: in any sywac execution, only one .run() handler is ever called. Once you know that, it's relatively easy to piece together which run handler will execute. Do we need errors? I don't know that we need them, but it might save users from some head-scratching. There's probably two places we could error that might be helpful:
|
Works for me! Let's give it a shot. |
SUMMARY
Broke this out from #63 so it can be discussed separately. The idea is to have a "run" handler for non-command CLIs, so that when the program's logic executes happens in the same place in either case.
EXAMPLE
DISCUSSION
Implementation of this feature would be very easy, but explaining it to a user might not be. The issue with supporting "run" (i.e.: what should this program do when it is parsed or parsedAndExit'd?) is that it conflicts with the command's run:
If I ran
myapp start
, which run function in this program should I expect to execute? It's not clear and I'm not sure how I would explain that to a user.The text was updated successfully, but these errors were encountered: