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

add context.Context to cli.Context #840

Merged
merged 8 commits into from
Aug 6, 2019
Merged

add context.Context to cli.Context #840

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
820040e
add context.Context to cli.Context
marwan-at-work Aug 5, 2019
006fad3
fix go.mod
marwan-at-work Aug 5, 2019
cee005e
only create context once
marwan-at-work Aug 6, 2019
1f7d168
fix nil check
marwan-at-work Aug 6, 2019
98e64f4
add propagation tests
marwan-at-work Aug 6, 2019
2fb0dc1
ignore compile errors
marwan-at-work Aug 6, 2019
c8863d0
PR updates
marwan-at-work Aug 6, 2019
be6dbba
rm modules
marwan-at-work Aug 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ func ExampleApp_Run_shellComplete() {
os.Args = []string{"greet", fmt.Sprintf("--%s", genCompName())}

app := &App{
Name: "greet",
Name: "greet",
EnableShellCompletion: true,
Commands: []*Command{
{
Expand Down Expand Up @@ -503,7 +503,6 @@ func TestApp_Float64Flag(t *testing.T) {
}

func TestApp_ParseSliceFlags(t *testing.T) {
var parsedOption, firstArg string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the Go command complained that these were declared and not used

Copy link
Contributor

Choose a reason for hiding this comment

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

These are not used but they do have side effects/assert evaluation logic so should probably be called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AudriusButkevicius the code didn't even compile though, so im not sure how that's supposed to work? In any case, I can just add var _ = parsedOption below

Copy link
Contributor

Choose a reason for hiding this comment

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

Its not the variable declarations, its the calls that assign to the unused variables is what matters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AudriusButkevicius again, the code doesn't even compile and tests don't run :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because we're just re-assigning those variables, but not really doing anything with them. It could be that go1.8 (what travis is using) didn't care, but Go 1.13 is smarter.

var parsedIntSlice []int
var parsedStringSlice []string

Expand All @@ -518,8 +517,6 @@ func TestApp_ParseSliceFlags(t *testing.T) {
Action: func(c *Context) error {
parsedIntSlice = c.IntSlice("p")
parsedStringSlice = c.StringSlice("ip")
parsedOption = c.String("option")
firstArg = c.Args().First()
return nil
},
},
Expand Down
18 changes: 17 additions & 1 deletion context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,22 @@
package cli

import (
"context"
"errors"
"flag"
"os"
"os/signal"
"reflect"
"strings"
"syscall"
)

// Context is a type that is passed through to
// each Handler action in a cli application. Context
// can be used to retrieve context-specific args and
// parsed command-line options.
type Context struct {
context.Context
App *App
Command *Command
shellComplete bool
Expand All @@ -24,9 +28,21 @@ type Context struct {
// NewContext creates a new context. For use in when invoking an App or Command action.
func NewContext(app *App, set *flag.FlagSet, parentCtx *Context) *Context {
c := &Context{App: app, flagSet: set, parentContext: parentCtx}

if parentCtx != nil {
if parentCtx.Context == nil {
parentCtx.Context = context.Background()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This has a side effect on the parentCtx passed in, which I as a user would find suprising.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think the intended condition in the if was == nil it seems?

I think this needs tests, as we can't rely on reviewers to prove this works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AudriusButkevicius this is just to make sure the context is never nil, so that we never panic. Should I remove it and leave the risk up to the user?

Copy link
Contributor

@AudriusButkevicius AudriusButkevicius Aug 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's not true, the if statement is wrong and it always overrides it if its NOT nil, and leaves it nil otherwise. I think we should not cause side effects on objects given to us, so if its nil, create a new one locally, and then validate this behaviour with a few tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AudriusButkevicius great catch, that was a mistake on my part. I will change != nil to == nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AudriusButkevicius i also added some propagation tests to ensure the parent context does not get overridden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to add sigterm tests, but i'm not sure how I can send a sigterm signal to the process, without killing the tests themselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think we should not mutate the parent context.

c.Context = parentCtx.Context

Then outside the if (oppose to else):

If c.Context == nil {
  // Create cancel context, spin up signal handler ctx.
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AudriusButkevicius done

c.Context = parentCtx.Context
c.shellComplete = parentCtx.shellComplete
} else {
ctx, cancel := context.WithCancel(context.Background())
go func() {
defer cancel()
sigs := make(chan os.Signal, 1)
signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
<-sigs
}()
c.Context = ctx
}

return c
Expand Down
22 changes: 22 additions & 0 deletions context_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cli

import (
"context"
"flag"
"sort"
"testing"
Expand Down Expand Up @@ -262,3 +263,24 @@ func TestContext_lookupFlagSet(t *testing.T) {
t.Fail()
}
}

// TestContextPropagation tests that
// *cli.Context always has a valid
// context.Context
func TestContextPropagation(t *testing.T) {
ctx := NewContext(nil, nil, nil)
if ctx.Context == nil {
t.Fatal("expected a non nil context when no parent is present")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a separate test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AudriusButkevicius I made this into a separate test

parent := NewContext(nil, nil, nil)
parent.Context = context.WithValue(context.Background(), "key", "val")
ctx = NewContext(nil, nil, parent)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about a test where parent.Context is nil, but parent not nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AudriusButkevicius i added that scenario to the bottom of this test.

val := ctx.Value("key")
if val == nil {
t.Fatal("expected a parent context to be inherited but got nil")
}
valstr, _ := val.(string)
if valstr != "val" {
t.Fatalf("expected the context value to be %q but got %q", "val", valstr)
}
}
3 changes: 3 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module gopkg.in/urfave/cli.v2

go 1.12
marwan-at-work marked this conversation as resolved.
Show resolved Hide resolved