Skip to content

Cleanup redundant code in App/Command #1498

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Oct 14, 2022
Merged

Cleanup redundant code in App/Command #1498

merged 13 commits into from
Oct 14, 2022

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

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