Revert "Merge pull request #741 from corruptmemory/expose-value"

[coilysiren]

Changes

This reverts commit 108d39a, reversing
changes made to 754ed1b.

What type of PR is this?

• bug

What this PR does / why we need it:

#741 (comment)

Testing

I haven't seen a reproduction for this issue, so @Napas if you're investing in seeing this merged you need to provide me with a test case.

Release Notes

- Removed changing context to expose a Value accessor from [#741](https://github.com/urfave/cli/pull/741)

[rliebz]

While it's unfortunate that a breaking change made it through in a minor release, I'm not sure reverting is the best option.

The breaking change was already released with v2.1.0, meaning we've gone 20 days with the repo published after things are technically already broken. From what I can tell, context.Context support was only implemented with v2.0.0 on Nov 21 (53 days ago), so we've spent roughly the same amount of time with each version of the code being the latest release.

This is long enough that I don't think reverting is the safer option. If we decide to revert that change, we're making a second breaking change for all consumers who have adopted the library since then.

In my mind, the question to ask is: Is the previous version of the code so much better that we're willing to make a breaking change to revert it?

If the answer is yes because the current state of the code is so fundamentally broken that it doesn't make sense to keep, then we should consider this reversion as a bug fix and proceed. If the answer is yes, but the current code isn't fundamentally broken, we're probably talking about a major release to change it back or find a new pattern that satisfies both use cases.

In the meantime, we do have released versions with either behavior, so pinning may be necessary for people who relied on the old interface.

[coilysiren]

I agree with all of that @rliebz! At the same time, I'm not personally invested in shepherding or advocating for this particular PR, so I'm going to close it ♻️

[asahasrabuddhe]

I was confused by this PR for a minute and am glad that it was closed. This is why I suggested pinning as the first option.

[coilysiren]

I created the reversion PR essentially because people dropping into PRs with 🚨 this broke my stuff! 🚨 is something I take very seriously, and I want to always try my best to help them out. TBH it's never clear to me when people will actually accept "just pin an old version" as an answer 🤷‍♀

[asahasrabuddhe]

It's very considerate of you @lynncyrin to try to accommodate this request but then there would always be a set of people who would remain unsatisfied with whatever we have done.

I agree with @rliebz that we have not introduced the breaking change on purpose. The PR in question was ages old and such kinds of things are bound to happen when resurrecting old libraries. I think that as maintainers we should have the right to take such a decision and advise people to just pin to a version when the cost of doing otherwise would be too large.

In this case, I think we can help @Napas get around with the breaking change by making his code work with the new update. I am all in for that :)

[Napas]

@lynncyrin I'm sorry if I came on too strong, I didn't intend to.

About PR. My main concerns are:

1. If an application or library has a context, I presume that it will implement default Golangs context.Context
2. It makes testing so much harder. My current approach was to make an interface for cli.Context, then, when testing action function, mock of the cli.Context can be passed into it and used as context.Context in other calls.

I can think about 2 possible solutions:

1. Revert changes, and instead Value(string) interface{} expose function with another name i.e. Interface(string) interface{}
2. Keep the changes bu change Value(string) interface{} to Value(interface{}) interface{}
3. Any other suggestions?

I will gladly do the changes for the PR as longs as there will be agreement on how to solve it.

[rliebz]

Unfortunately, both of those suggested changes break the current API, so they're probably a no-go until a v3 release. I think the first is probably the one we should have gone with in the original PR if we had realized the implications of the change.

It makes testing so much harder. My current approach was to make an interface for cli.Context, then, when testing action function, mock of the cli.Context can be passed into it and used as context.Context in other calls.

I agree with this completely. From what I can tell, there's currently no good way to generate a test cli.Context that covers the use cases we'd want to test. If we're talking about v3 breaking changes, I'd prioritize this pretty highly on my list of goals.

One thing I've done in the past is making an interface of the subset of cli.Context that my action function needs. If you do that, you can mock it in a test and cover the important logic:

// context represents the subset of *cli.Context required for flag completion.
//
// This is just an example of only needing a couple methods, but you could pick any subset.
type context interface {
	IsSet(string) bool
	NArg() int
}

// This can be a method on a struct or accept args, if needed
func createActionFunc() func(c *cli.Context) error {
	return func(c *cli.Context) error {
		// actionFunc can accept any number of args, but the important thing
		// is to accept a `context` interface, not a `*cli.Context`
		return actionFunc(c)
	}
}

// Since the actionFunc will have all our real application logic, this is what I'd be
// most interested in testing. Because `context` is an interface here, that's simple
func actionFunc(c context) error {
		// ...
}

Then for tests, you can stub behavior how ever you want:

type mockContext struct {
	narg  int
	flags []string
}

func (m mockContext) NArg() int {
	return m.narg
}

func (m mockContext) IsSet(name string) bool {
	for _, flag := range m.flags {
		if flag == name {
			return true
		}
	}

	return false
}

If you need to access non-method members like c.App or c.Command, you could build a lightweight wrapper to be mockable:

type mockableContext struct {
	*cli.Context
}

func (m mockableContext) GetApp() *cli.App {
	return m.Context.App
}

func (m mockableContext) GetCommand() *cli.Command {
	return m.Context.Command
}

type context interface {
	GetApp() *cli.App
	GetCommand() *cli.Command
}

And passing it in is trivial:

func(c *cli.Context) error {
  m := mockableContext{c}
  return actionFunc(m)
}

Let me know if there are use cases that doesn't really cover. The more context I have around the patterns you're following, the more I might be able to suggest, but I think we're stuck with the current API for now.