urfave / cli

A simple, fast, and fun package for building command line apps in Go
https://cli.urfave.org
MIT License
21.91k stars 1.69k forks source link

Collapse `Context` into `Command` #1784

Closed meatballhat closed 1 year ago

meatballhat commented 1 year ago

What type of PR is this?

(REQUIRED)

What this PR does / why we need it:

(REQUIRED)

Which issue(s) this PR fixes:

Closes #1587 Closes #1780

Special notes for your reviewer:

(fill-in or delete this section)

Testing

(fill-in or delete this section)

Release Notes

(REQUIRED)

github-actions[bot] commented 1 year ago

What is Frogbot?

skelouse commented 1 year ago

Quick thought: What if rather than passing (ctx.Context, cmd) to Action, Before, and After, pass a context.WithValue? Then for example rather than calling ctx.String("<flag>") you'd use <import>.String(ctx, "<flag>"). Although this would simply be flipping storing context within the urfave/cli's context to storing package context within context.Context and may complicate unit testing.

Also, will command be passed to flag Action? Action func(context.Context, cmd, T) error I see: return s.BoolFlag.Action(ctx, cmd, s.Value())

meatballhat commented 1 year ago

Quick thought: What if rather than passing (ctx.Context, cmd) to Action, Before, and After, pass a context.WithValue? Then for example rather than calling ctx.String("<flag>") you'd use <import>.String(ctx, "<flag>"). Although this would simply be flipping storing context within the urfave/cli's context to storing package context within context.Context and may complicate unit testing.

@skelouse I'm pretty nervous about API signatures that intentionally pass values through context.Context's Value func. Is that what you meant?

skelouse commented 1 year ago

@skelouse I'm pretty nervous about API signatures that intentionally pass values through context.Context's Value func. Is that what you meant?

Yeah that's what I meant.

meatballhat commented 1 year ago

@skelouse I'm pretty nervous about API signatures that intentionally pass values through context.Context's Value func. Is that what you meant?

Yeah that's what I meant.

Yeeeeahh I'd like to avoid that if possible 😅 I feel nervous enough about using a context.Context value to track the internal relationship between a *Command and it's potential parent *Command.

github-actions[bot] commented 1 year ago


[This comment was generated by JFrog Frogbot 🐸](https://github.com/jfrog/frogbot#readme)