add context.Context to cli.Context #840
Conversation
go.mod
Outdated
| @@ -0,0 +1,3 @@ | |||
| module gopkg.in/urfave/fli.v2 | |||
There was a problem hiding this comment.
Fli? Also, why gopkg.in?
There was a problem hiding this comment.
@AudriusButkevicius ops, will fix that. What import path should it be? The README for v2 says use gopkg.in which is why I added that
There was a problem hiding this comment.
I guess this needs a wider discussion on how we deal with this going forward. With go.mod support I think gopkg.in is obsolete, and people just go get github.com/urfave/cli@2.0.0
We can keep releasing 1.0.0 and 2.0.0 idependently.
ping @lynncyrin
There was a problem hiding this comment.
@AudriusButkevicius github.com/urfave/cli@2.0.0 would not work. Per "Semantic Import Versioning" it would have to be github.com/urfave/cli/v2 and similarly go get github.com/urfave/cli/v2@v2.1.9 and so on.
Note that Go Modules has a special handling of Gopkg.in so it should work okay. That said, I'm happy to remove the go.mod file from this PR and open a new issue for what the import path should be/
There was a problem hiding this comment.
Why would @2.0.0 not work, I was under the assumption that just resolves the release tag. Not sure why it needs to move to a separate subpackage to work?
There was a problem hiding this comment.
@AudriusButkevicius go get github.com/urfave/cli will always fetch v1, and go get github.com/urfave/cli/v2 will always fetch v2 and so on.
There was a problem hiding this comment.
Use, unless we merge v2 to master and roll with that. I don't think two versions of the library should exist in the same repo basically.
There was a problem hiding this comment.
@AudriusButkevicius Go Modules does not care about what's in master, when you have semver tags. It will basically look at git tags and determine what version to get you.
There was a problem hiding this comment.
@AudriusButkevicius for this PR however, should I just remove the go.mod file? Or should I change it to be github.com/urfave/cli/v2?
There was a problem hiding this comment.
No idea, thats why I want someone else from @urfave/cli to comment and drive the direction of the migration.
context.go
Outdated
| if parentCtx != nil { | ||
| if parentCtx.Context != nil { | ||
| parentCtx.Context = context.Background() | ||
| } |
There was a problem hiding this comment.
This has a side effect on the parentCtx passed in, which I as a user would find suprising.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@AudriusButkevicius great catch, that was a mistake on my part. I will change != nil to == nil
There was a problem hiding this comment.
@AudriusButkevicius i also added some propagation tests to ensure the parent context does not get overridden.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
}
There was a problem hiding this comment.
@AudriusButkevicius done
app_test.go
Outdated
| @@ -503,7 +503,6 @@ func TestApp_Float64Flag(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestApp_ParseSliceFlags(t *testing.T) { | |||
| var parsedOption, firstArg string | |||
There was a problem hiding this comment.
the Go command complained that these were declared and not used
There was a problem hiding this comment.
These are not used but they do have side effects/assert evaluation logic so should probably be called?
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
Its not the variable declarations, its the calls that assign to the unused variables is what matters.
There was a problem hiding this comment.
@AudriusButkevicius again, the code doesn't even compile and tests don't run :)
There was a problem hiding this comment.
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.
| ctx := NewContext(nil, nil, nil) | ||
| if ctx.Context == nil { | ||
| t.Fatal("expected a non nil context when no parent is present") | ||
| } |
There was a problem hiding this comment.
Should be a separate test
There was a problem hiding this comment.
@AudriusButkevicius I made this into a separate test
context_test.go
Outdated
| } | ||
| parent := NewContext(nil, nil, nil) | ||
| parent.Context = context.WithValue(context.Background(), "key", "val") | ||
| ctx = NewContext(nil, nil, parent) |
There was a problem hiding this comment.
What about a test where parent.Context is nil, but parent not nil.
There was a problem hiding this comment.
@AudriusButkevicius i added that scenario to the bottom of this test.
|
lgtm |
|
Oh rather, LGTM pending the tests passing 🙂 |
|
@lynncyrin I'm not sure why the build is failing. It has to do with the python script which im not familiar with. Can you please point me in the right direction to fix this? Thanks |
|
I think the test is a red herring, we need to bump travis version. |
This is a naive implementation of attaching context.Context to the *cli.Context
The goroutine that waits for a kill signal hangs forever. I imagine that's okay since running a cli command does not hang forever. Otherwise, I'd be happy to add a way to end the goroutine.
Fixes #839