Skip to content

Cleanup redundant code in App/Command#1498

Merged
dearchap merged 13 commits intourfave:mainfrom
dearchap:optimize_command_run
Oct 14, 2022
Merged

Cleanup redundant code in App/Command#1498
dearchap merged 13 commits intourfave:mainfrom
dearchap:optimize_command_run

Conversation

@dearchap
Copy link
Contributor

@dearchap dearchap commented Sep 20, 2022 •
edited
Loading

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 force-pushed the optimize_command_run branch from c4780b8 to 748ea87 Compare September 25, 2022 01:07
@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
Closed
18 tasks
@dearchap dearchap force-pushed the optimize_command_run branch from dab5f01 to adcce13 Compare October 9, 2022 21:51
meatballhat
meatballhat requested changes Oct 10, 2022
View reviewed changes
@meatballhat
Copy link
Member

meatballhat commented Oct 10, 2022

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!\" != \"\""

meatballhat
meatballhat reviewed Oct 10, 2022
View reviewed changes
meatballhat
meatballhat previously approved these changes Oct 14, 2022
View reviewed changes
@dearchap @meatballhat
Update app.go
33d08c3 
Co-authored-by: Dan Buch <dan@meatballhat.com>
@dearchap dearchap merged commit 0d0d329 into urfave:main Oct 14, 2022
@renovate renovate bot mentioned this pull request Jul 21, 2025
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@meatballhat meatballhat meatballhat left review comments

Assignees

No one assigned

Labels

area/v2 relates to / is being considered for v2 kind/cleanup describes internal cleanup / maintaince kind/discussion is for discussion

Projects

None yet

Milestone

Release 2.x

Development

Successfully merging this pull request may close these issues.

Refactor App and Command to share logic

2 participants

@dearchap @meatballhat