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

Consolidate all App code into Command #1602

Closed
wants to merge 10 commits into from
Closed

Conversation

meatballhat
Copy link
Member

@meatballhat meatballhat commented Nov 28, 2022

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:

(REQUIRED)

Fixes #1586

Special notes for your reviewer:

(fill-in or delete this section)

Testing

(fill-in or delete this section)

Release Notes

(REQUIRED)


@dearchap
Copy link
Contributor

@meatballhat Perhaps its better to split this into two separate PRs. One to change Subcommands -> Commands and another to merge App into command.

@meatballhat
Copy link
Member Author

@meatballhat Perhaps its better to split this into two separate PRs. One to change Subcommands -> Commands and another to merge App into command.

@dearchap that was my intention when I started, but keeping both .Commands and .Subcommands didn't seem feasible 🤔 Are you suggesting I take on the .Subcommands ➡️ .Commands rename first?

@dearchap
Copy link
Contributor

dearchap commented Dec 6, 2022

@meatballhat One thing that may help is removing ctx from Help and using Command params throughout. That way there are no App/Ctx references anywhere on that portion. If you'd like I can work on that.

@meatballhat
Copy link
Member Author

@meatballhat One thing that may help is removing ctx from Help and using Command params throughout. That way there are no App/Ctx references anywhere on that portion. If you'd like I can work on that.

@dearchap that sounds great to me! 🎉

help_test.go Outdated
@@ -574,58 +574,6 @@ func TestShowCommandHelp_CommandAliases(t *testing.T) {
}
}

func TestHelpNameConsistency(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@meatballhat Any reason why this was removed ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh gosh I totally don't remember why I did this 😭 I'm starting over!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, I remember! But I'm still starting over 😂 I'll be introducing smaller PRs to do parts of what's discovered here.

@meatballhat
Copy link
Member Author

Superseded by #1746

@meatballhat meatballhat closed this Jun 6, 2023
@meatballhat meatballhat deleted the app-command-collapse branch June 6, 2023 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consolidate all App code into Command
3 participants