Skip to content
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

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

Closed
AnthonyPoschen opened this issue Feb 19, 2019 · 10 comments
Closed

Comments

@AnthonyPoschen
Copy link

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
Copy link
Owner

akamensky commented Feb 20, 2019

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:

  • make breaking changes (and some other people would suffer)
  • wait until proper dependency management is rolled out in Go (modules are still WIP at the moment) and then have breaking changes go into v2 of this library
  • make changes backward compatible in the sense that help command is optional, but enabled by default (and can be disabled in code)

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

@AnthonyPoschen
Copy link
Author

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
Copy link

hongcai commented May 20, 2019

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
Copy link
Owner

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 added a commit to densestvoid/argparse that referenced this issue Oct 2, 2019
Designed to begin addressing issue akamensky#29

HelpFunc added as field for Parsers and Commands
Parser HelpFunc defaults to Usage unless overwritten
Command HelpFunc defaults to first non nil parent HelpFunc

Design choice to enable backwards compatibility required
exposing Command and Arg fields with Getter functions.

Does not resolve:
	exiting program on help invocation
	overwrritting/disabling -h | --help argument strings
	enabling other argument strings to invoke help
densestvoid added a commit to densestvoid/argparse that referenced this issue Oct 2, 2019
Designed to begin addressing issue akamensky#29

HelpFunc added as field for Parsers and Commands
Parser HelpFunc defaults to Usage unless overwritten
Command HelpFunc defaults to first non nil parent HelpFunc

Design choice to enable backwards compatibility required
exposing Command and arg fields with Getter functions and an interface.

Does not resolve:
	exiting program on help invocation
	overwrritting/disabling -h | --help argument strings
	enabling other argument strings to invoke help
densestvoid added a commit to densestvoid/argparse that referenced this issue Oct 18, 2019
Adds a Settings struct for creating parsers/commands with
NewParserWithSettings or NewCommandWithSettings.

Settings
	HelpDisabled -	defaults false, set true to not generate a help
			argument for parser/command
	HelpSname -	short name for the parser/command help argument.
			Can be left empty
	HelpLname -	long name for the parser/command help argument.
			Leaving empty forces use of -h/--help when
			HelpDisabled is false
	NoExitOnHelp -	defaults false, set true to not exit on help
			argument being parsed

Should resolve all outstanding help argument issues.
@densestvoid
Copy link
Contributor

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.

densestvoid added a commit to densestvoid/argparse that referenced this issue Dec 9, 2019
Fixed merge conflict error due to check function return type change confusion.
@akamensky
Copy link
Owner

@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
Copy link
Contributor

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

@densestvoid densestvoid mentioned this issue Jan 25, 2020
akamensky pushed a commit that referenced this issue Jan 29, 2020
* Finishes addressing issue #29

Adds a Settings struct for creating parsers/commands with
NewParserWithSettings or NewCommandWithSettings.

Settings
	HelpDisabled -	defaults false, set true to not generate a help
			argument for parser/command
	HelpSname -	short name for the parser/command help argument.
			Can be left empty
	HelpLname -	long name for the parser/command help argument.
			Leaving empty forces use of -h/--help when
			HelpDisabled is false
	NoExitOnHelp -	defaults false, set true to not exit on help
			argument being parsed

Should resolve all outstanding help argument issues.

* Finishing Issue #29

Fixed merge conflict error due to check function return type change confusion.

* Updated Settings Object to multiple function calls

Added functions:
	DisableHelp
	SetHelp
	ExitOnHelp

Added Command.exitOnHelp

Added more tests to increase code coverage

* Updated no help example
@akamensky
Copy link
Owner

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

@densestvoid
Copy link
Contributor

@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
Copy link
Owner

Closing, please re-open if not resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants