Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collapse Context into Command #1784

Merged
merged 10 commits into from
Jun 29, 2023
Merged

Collapse Context into Command #1784

merged 10 commits into from
Jun 29, 2023

Conversation

meatballhat
Copy link
Member

What type of PR is this?

(REQUIRED)

  • bug
  • cleanup
  • documentation
  • feature

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)


@meatballhat meatballhat changed the title First pass collapsing Context into Command Collapse Context into Command Jun 25, 2023
@dearchap dearchap marked this pull request as ready for review June 27, 2023 06:44
@dearchap dearchap requested a review from a team as a code owner June 27, 2023 06:44
@meatballhat meatballhat temporarily deployed to frogbot June 27, 2023 13:29 — with GitHub Actions Inactive
@github-actions
Copy link

What is Frogbot?

@skelouse
Copy link
Contributor

skelouse commented Jun 27, 2023

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
Copy link
Member Author

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
Copy link
Contributor

skelouse commented Jun 27, 2023

@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
Copy link
Member Author

meatballhat commented Jun 28, 2023

@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.

flag_impl.go Outdated Show resolved Hide resolved
@meatballhat meatballhat temporarily deployed to frogbot June 28, 2023 22:36 — with GitHub Actions Inactive
@github-actions
Copy link


  • Frogbot also supports Contextual Analysis, Infrastructure as Code Scanning and Secrets Detection. These features are included as part of the JFrog Advanced Security package, which isn't enabled on your system.

@meatballhat meatballhat added this to the Release 3.x milestone Jun 28, 2023
@meatballhat meatballhat merged commit fbb9421 into main Jun 29, 2023
12 checks passed
@meatballhat meatballhat deleted the collapse-context-command branch June 29, 2023 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants