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.

Sign up for GitHub

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

Jump to bottom

Custom help #50

Merged
merged 6 commits into from
Jan 29, 2020
Merged

Custom help #50

merged 6 commits into from
Jan 29, 2020

Conversation

densestvoid
Copy link
Contributor

Finishes addressing issue. Removed check function to reduce repetitive code and enable extended help argument handling.

@densestvoid
Finishes addressing issue akamensky#29
0f6d107 
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
Merge remote-tracking branch 'upstream/master' into custom-help
9da2c2e
@densestvoid
Finishing Issue akamensky#29
d5a30b0 
Fixed merge conflict error due to check function return type change confusion.
akamensky
akamensky reviewed Dec 10, 2019
View reviewed changes
argparse.go Outdated Show resolved Hide resolved
akamensky
akamensky reviewed Dec 10, 2019
View reviewed changes
argparse.go Outdated Show resolved Hide resolved
akamensky
akamensky reviewed Dec 10, 2019
View reviewed changes
Copy link
Owner

@akamensky akamensky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR, I found couple of small typos. Also please see review comment on having a single method NewCommand instead of two.

argparse.go Outdated Show resolved Hide resolved
argument.go Outdated Show resolved Hide resolved
argument.go Outdated Show resolved Hide resolved
@akamensky
Copy link
Owner

Also note, that this decreases test coverage which currently fails the merge checks. Some newly added code is not covered by tests.

@densestvoid densestvoid mentioned this pull request Jan 23, 2020
Closed
@akamensky
Copy link
Owner

@densestvoid

I feel this is rather complex PR that adds unnecessary new methods to the parser (and commands).

How do you feel instead of taking this approach to have a method that simply overrides default behavior.

For example:

p := argparse.NewParser("", "")
p.SetHelpSettings("h", "help", false, true)

where function signature would be something like:

func (...) SetHelpSettings(sname, lname stirng, disabled, noExit bool)

Also this PR shows a decrease in test coverage, which is set to fail merge checks.

@densestvoid
Copy link
Contributor Author

Seems like a reasonable solution, I'll adapt what I currently have and add code coverage to pass validation.

@densestvoid
Updated Settings Object to multiple function calls
f720dad 
Added functions:
	DisableHelp
	SetHelp
	ExitOnHelp

Added Command.exitOnHelp

Added more tests to increase code coverage
@densestvoid
Merge branch 'master' into custom-help
8cd9975
@densestvoid
Updated no help example
44b8b28
@densestvoid
Copy link
Contributor Author

@akamensky Ready for review. I took your suggested SetHelpSettings functions and split it. Please let me know if anything needs changed. If this PR is satisfactory, it would solve issue #29 .

@akamensky akamensky merged commit 6876ef2 into akamensky:master Jan 29, 2020
@akamensky
Copy link
Owner

Looks good. Merged.

@densestvoid densestvoid deleted the custom-help branch May 7, 2020 15:01
@sayap sayap mentioned this pull request Dec 14, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akamensky akamensky akamensky left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@densestvoid @akamensky