Exposed the value accessor in Context

[corruptmemory]

I wrote a small utility to populate a struct reflectively from command-line-arguments, but to do this I needed to get at the values stored via value.

[AudriusButkevicius]

Not sure why? I've written something simillar and not sure I needed that.

[coilysiren]

👋 @corruptmemory if you're still interested in this PR, can you respond to AudriusButkevicius's comment above? If not - feel free to ignore this ping, and we can close the PR so it won't become stale.

[AudriusButkevicius]

actually, it's not a unreasonable change.

I am using reflection on the destination, and then based on that I pick the "getter", so it's reasonable to want to do the inverse.

[corruptmemory]

Nice to see some life coming to the PR. I'll rebase and push sometime this weekend.

[coilysiren]

Oh no, this PR has been assailed by merge conflicts! @corruptmemory can ya take care of that?

[coilysiren]

👋 heya, I think I'll close this within the next ~week if you don't have time for it @corruptmemory! You are very much encouraged to re-open it later, though 🙏

[corruptmemory]

Sorry for the late response. I'll clean this up this week.

[codecov]

Codecov Report

Merging #741 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #741   +/-   ##
=======================================
  Coverage   72.89%   72.89%           
=======================================
  Files          32       32           
  Lines        2439     2439           
=======================================
  Hits         1778     1778           
  Misses        550      550           
  Partials      111      111

Impacted Files	Coverage Δ
context.go	94.16% <100%> (ø)	⬆️

---

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 754ed1b...615e70e. Read the comment docs.

[corruptmemory]

I see that there is now a collision between calling (*context.Context).Value(interface{}) and (*cli.Context).Value(string) and the second one is winning causing the NPE.

This change in line 331 in context_test.go seems to address the issue:

val := ctx.Context.Value("key")

[saschagrunert]

I think this makes sense. LGTM

[Napas]

@lynncyrin This PR introduces breaking changes.

Value(string) interface{} overrides Value(interface{}) interface{} from the context.Context and cli.Context stops implementing context.Context interface.

Please revert it ASAP.

[asahasrabuddhe]

@Napas Are you using a dependency management system? I think not. I would suggest that pin to a release prior to v2.1.0 instead of getting the latest code directly from the master.

[Napas]

@Napas Are you using a dependency management system? I think not. I would suggest that pin to a release prior to v2.1.0 instead of getting the latest code directly from the master.

I am doing that until it will be reverted, but minor version release should not have breaking changes.

On top of that, it makes testing much harder as cli.Context can't have mocked and used as context.Context downstream.

[coilysiren]

I'll revert this now, but just so it's 100% clear - there's no specific reason why reverting this should be blocked on me. Anyone can submit a reversion PR.

[coilysiren]

@Napas @asahasrabuddhe => #1039

[coilysiren]

I am doing that until it will be reverted, but minor version release should not have breaking changes

Just in case anyone isn't aware - by no means did anyone involved in this PR merge a breaking change on purpose.