-
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
add RunWithContext + remove signal cancellation #975
Conversation
Codecov Report
@@ Coverage Diff @@
## master #975 +/- ##
==========================================
+ Coverage 73.36% 73.38% +0.02%
==========================================
Files 32 32
Lines 2440 2435 -5
==========================================
- Hits 1790 1787 -3
+ Misses 540 539 -1
+ Partials 110 109 -1
Continue to review full report at Codecov.
|
@@ -224,7 +232,7 @@ func (a *App) Run(arguments []string) (err error) { | |||
|
|||
err = parseIter(set, a, arguments[1:], shellComplete) | |||
nerr := normalizeFlags(a.Flags, set) | |||
context := NewContext(a, set, nil) | |||
context := NewContext(a, set, &Context{Context: ctx}) |
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.
Will this still work of user does RunWithContext(nil, args)
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.
Passing a nil Context is always a developer error.
Do not pass a nil Context, even if a function permits it. Pass context.TODO if you are unsure about which Context to use.
We shouldn't need to support that
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.
LGTM, pending RunWithContext
-> RunContext
@rliebz @AudriusButkevicius PR updated :) |
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.
LGTM
Fixes #945