-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Remove NewApp
func
#1626
Remove NewApp
func
#1626
Conversation
in favor of focusing on declarative API. Supports #1586
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m all for declarative usage
@@ -574,58 +574,6 @@ func TestShowCommandHelp_CommandAliases(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestHelpNameConsistency(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happened to this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's no longer necessary or valid given that NewApp
is removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No no no. This is to test whether the help occurs consistently between
app help cmd
app cmd help
app cmd -h
app cmd subcmd help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The inconsistency shows up when users use NewApp() as opposed to | ||
// using App{...} directly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dearchap sorry I was interpreting this comment to mean that since NewApp
is removed that testing the inconsistency was no longer valid 🤔
What type of PR is this?
What this PR does / why we need it:
in favor of focusing on declarative API.
Which issue(s) this PR fixes:
Supports #1586