akamensky / argparse

Argparse for golang. Just because `flag` sucks
MIT License
604 stars 62 forks source link

Help Command closes Program - undesirable in web chat app parsing #29

Closed AnthonyPoschen closed 4 years ago

AnthonyPoschen commented 5 years ago

Hi i am trying to build a discord chat bot, and leveraging this package has been awesome for that, however when the parser is created a non overridable help argument is added which os.Exit(0) when triggered which is meaning users chatting the bot can make it close. I know this is probably a unexpected use case, i am willing to fork the project and workout a solution if you would like to give my guidance on how the end state should look.

akamensky commented 5 years ago

Hi @zanven42

I totally see where you are coming from. I have also considered that perhaps having -h|--help always defined is a possible issue and that should probably be either optional or left up to developer to implement.

However I am reluctant to change it now as this may introduce breaking changes. And I see quite a few people using this in their projects already.

I think we have 3 options here:

I personally like 3rd option better since it will satisfy your issue and also keep things as they currently are for others.

AnthonyPoschen commented 5 years ago

yeah, i think option 3 would be best to continue on. While i was hacking away last night on a discord bot i forked the repository and what i had changed was that the check call so it also returned an error not just bool, when -h|--help was found that it returned an error for parse, but that made me have to tree dive the happened calls on sub commands to know which subcommand usage i needed in the error block.

i'll see what i can come up with that is backwards compatible because i definitely like the Help object but just want the text output for me to consume and no os.exit.

what i was thinking is when parse detects that help has been called it returns an error saying something like "Help called" or something more suitable, but internally track where we hit help so when usage is called we can return the correct sub commands usage. I don't know too much yet about the internals and i am not sure if that is a separate issue or caused from my hacky fix.

hongcai commented 5 years ago

That would be great, if the -h|--help description is configurable. Since sometimes we like to make it more local by using our local language.

akamensky commented 5 years ago

Agree, but I would like to keep it backward compatible to avoid unexpected surprises to those who always go get -u -v -x this library.

I think good start at this issue would be to move default help message formatting into its own exported method, and use that method as default option for help message. Which then can be overwritten with any other method.

After that can see how can make that -h|--help can be disabled (with default being enabled) or overwritten with other flags. Perhaps another exported method on Parser that would allow setting arguments that will invoke help message, while current -h|--help is used as default.

I would appreciate a PR for this since I am rather swamped with work at the moment.

densestvoid commented 4 years ago

Commit 0f6d107: Considered a Settings field HelpParseFunc in place of NoExitOnHelp. HelpParseFunc would be called when the help argument is parsed. If not set, HelpParseFunc would default to printing parser/command.Help(nil) and exiting (what currently happens). Users could define the HelpParseFunc to define how the parser should handle the help argument, and return a non nil error to stop parsing. I went with NoExitOnHelp because it solves the immediate problem, and is easier on the end user. I can create another branch to demonstrate if desired. HelpParseFunc would have the following signature: type HelpParseFunc func(*arg, string) error where arg is the help argument, string is the help text returned by *arg.parent.Help(nil), and error is expected to be non-nil to cancel parsing after handling the help argument.

akamensky commented 4 years ago

@densestvoid has this been resolved? (i haven't had a chance to test functionality just yet, but since you helped to implement this I presume you tested and possibly are using it)

densestvoid commented 4 years ago

50 Resolves these issues, but I was waiting for your input on the review

akamensky commented 4 years ago

@densestvoid the #50 is merged, feel free to close this issue if it is resolved in that PR. <3

densestvoid commented 4 years ago

@akamensky I do not have the access to close issues. Unless @zanven42 feels the recent help features do not fully address the issue, I would consider it closed.

akamensky commented 4 years ago

Closing, please re-open if not resolved.