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

Cleanup redundant code in App/Command #1498

Merged
merged 13 commits into from Oct 14, 2022

Conversation

dearchap
Copy link
Contributor

@dearchap dearchap commented Sep 20, 2022

What type of PR is this?

(REQUIRED)

  • cleanup

What this PR does / why we need it:

(REQUIRED)

Current code path requires almost similar code is three separate places. App.RunContext, App.RunAsSubcommand, and Command.Run. Any bug in flag handling/help etc would need changes in all these places. To make it more manageable the proposal is to condense it into a single place in Command.Run. To enable this we simulate the main App instance as a "rootCommand".

This is a draft proposal. It still requires some work to have all unit tests pass but I wanted some buy in from the community

Which issue(s) this PR fixes:

(REQUIRED)

Fixes #554

Special notes for your reviewer:

(fill-in or delete this section)

Testing

(fill-in or delete this section)

Release Notes

(REQUIRED)


@dearchap dearchap added kind/discussion is for discussion area/v2 relates to / is being considered for v2 kind/cleanup describes internal cleanup / maintaince labels Sep 20, 2022
@dearchap dearchap added this to the Release 2.x milestone Sep 20, 2022
@dearchap dearchap changed the title Initial cut Cleanup redundant code in App/Command Sep 20, 2022
@dearchap dearchap marked this pull request as ready for review September 29, 2022 14:35
@dearchap dearchap requested a review from a team as a code owner September 29, 2022 14:35
@dearchap dearchap mentioned this pull request Oct 1, 2022
20 tasks
command.go Outdated Show resolved Hide resolved
@meatballhat
Copy link
Member

fwiw the test-docs check failure seems to be revealing something that's unhappy somewhere

https://github.com/urfave/cli/actions/runs/3215687734/jobs/5256958204#step:7:244

# ---- >8 ----
time="2022-10-09T21:52:33Z" level=info msg=done error_count=1 example_count=29 fields.time=13.723986848s source_count=15
time="2022-10-09T21:52:33Z" level=error msg="expected output does not match actual: \"made it!\\nPhew!\" != \"\""

app_test.go Outdated Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
help_test.go Outdated Show resolved Hide resolved
meatballhat
meatballhat previously approved these changes Oct 14, 2022
app.go Outdated Show resolved Hide resolved
Co-authored-by: Dan Buch <dan@meatballhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v2 relates to / is being considered for v2 kind/cleanup describes internal cleanup / maintaince kind/discussion is for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor App and Command to share logic
2 participants