Extract cli.Context into an interface

[asahasrabuddhe]

What type of PR is this?

• cleanup

What this PR does / why we need it:

In response to the discussion here (https://github.com/urfave/cli/issues/833#issuecomment-574171962) this is an attempt to try to extract the cli.Context into its own interface. This is currently a WIP.

[codecov[bot]]

Codecov Report

Merging #1052 into v3-development-master will increase coverage by 0.69%. The diff coverage is 92.05%.

@@                    Coverage Diff                    @@
##           v3-development-master    #1052      +/-   ##
=========================================================
+ Coverage                   72.6%   73.29%   +0.69%     
=========================================================
  Files                         33       33              
  Lines                       2482     2494      +12     
=========================================================
+ Hits                        1802     1828      +26     
+ Misses                       569      558      -11     
+ Partials                     111      108       -3

Impacted Files	Coverage Δ
flag_timestamp.go	32.75% <0%> (ø)	:arrow_up:
altsrc/yaml_file_loader.go	42.85% <100%> (ø)	:arrow_up:
flag_int.go	70% <100%> (ø)	:arrow_up:
flag_string.go	88.88% <100%> (ø)	:arrow_up:
flag_duration.go	70% <100%> (ø)	:arrow_up:
flag_generic.go	56.75% <100%> (ø)	:arrow_up:
flag_string_slice.go	79.66% <100%> (+2.61%)	:arrow_up:
flag_int_slice.go	74.62% <100%> (+2.16%)	:arrow_up:
flag_int64.go	62.5% <100%> (ø)	:arrow_up:
command.go	82.57% <100%> (-0.14%)	:arrow_down:
... and 13 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f1a114a...3872a98. Read the comment docs.

[asahasrabuddhe]

@lynncyrin / @rliebz / @saschagrunert I would like to start a discussion over here about if you agree with the direction of this PR and what you would like to be changed

[asahasrabuddhe]

Thanks for the quick feedback guys especially on a Sunday! Let me get back to work and come back with some improvements :)

[asahasrabuddhe]

@saschagrunert / @rliebz I have updated the code to include some of our discussed changes. Please review :)

@lynncyrin Although GitHub not selecting you as a reviewer for this PR, I would really appreciate if you could review this when time permits. Thanks!

[coilysiren]

If this is the case, what's the benefit in making cli.Context an interface?

☝️ I'm interested in the answer to this question! From here https://github.com/urfave/cli/pull/1052#discussion_r373327116

[Nokel81]

Would it be possible instead of having a Context() context.Context method, to just have cli.Context extend context.Context? This would make using cli.Context easier.

The only problem with this is that the current Value(key string) interface{} method from type FlagValues interface conflicts with the Value(key interface{}) interface{} from context.Context. A possible solution to this could be making our function FlagValue(key string) interface{} instead.

[rliebz]

Would it be possible instead of having a Context() context.Context method, to just have cli.Context extend context.Context?

The challenge with this is that the context interface was not designed to be extended. Consider a CustomContext type that extends the context.Context interface, which I'll use as a stand-in for the cli.Context type. What happens if we try to add a timeout, for example?

package main

import (
	"context"
	"fmt"
	"time"
)

type CustomContext struct {
	context.Context
}

func main() {
	var ctx context.Context = CustomContext{context.TODO()}
	fmt.Printf("Type Before: %T\n", ctx)
	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
	defer cancel()
	fmt.Printf("Type After: %T\n", ctx)
}

This will print the following:

Type Before: main.CustomContext
Type After: *context.timerCtx

We create a new context that extends our previous one with no clear way to get our original type back. This means any custom functionality we have on our cli.Context is gone as soon as we try to perform operations on the context such as attaching deadlines or any ContextWithFoo-type methods that are common in third party libraries.

The problem we're solving is not dissimilar to the problem that the http package solves with http.Request—we have a struct that was already passed around from function to function containing metadata, and attaching a context.Context as a member of that object is probably the easiest way to work with it.

[stale[bot]]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[stale[bot]]

Closing this as it has become stale.

[meatballhat]

@asahasrabuddhe I'm interesting in continuing this work if you're not able ✋🏼

[asahasrabuddhe]

@meatballhat I can and would love to continue this. However, we need to decide on what direction to proceed. Based on earlier feedback, this PR was stuck. What do you think we should do?

[meatballhat]

@asahasrabuddhe Hello! I'm so glad you're here and willing and able to continue this work! After more carefully reviewing the commentary, I think I am generally in agreement with @rliebz, which I'd restate (please correct me @rliebz!) as:

• trivially creating a new and valid cli.Context is mostly the concern of this library's maintainers
• the benefits of defining an interface that allows alternative implementations are unclear or otherwise not strongly evident based on real world library usage

and also notable, mentioned in another comment:

• standard library context.Context was not built with extension in mind

All of which is to say that I think redefining cli.Context as an interface won't improve the experience of real world production library usage.

What do you think about instead building out a sub-package that specifically targets testing concerns, like what is available in net/http/httptest? For example:

// moved from current location
cCtx := clitest.NewContext(myApp, myFlagSet, nil)

// ...

cCtx := clitest.NewParentContext()

// ...

cCtx := clitest.NewContextWithFlagSet(myFlagSet)

[dearchap]

@asahasrabuddhe @meatballhat I'm closing this PR as we dont want to proceed in this direction. We can have a separate PR for subpackage