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

feature: add DefaultCommand field to App #1388

Merged
merged 5 commits into from Jun 24, 2022

Conversation

jalavosus
Copy link
Contributor

See issue #1307 for context.

What type of PR is this?

  • feature

What this PR does / why we need it:

Enables a CLI application to run a default Command (by name) if no command name is passed as a cli argument.

Which issue(s) this PR fixes:

Fixes #1307

Special notes for your reviewer:

Genuinely unsure if I implemented this correctly, but the test cases look sane on my end and they pass.

Release Notes

cli.App now has a `DefaultCommand` field, which, if set, enables a cli.App to run a specific command if no command is otherwise passed as an argument. 

@jalavosus jalavosus requested a review from a team as a code owner May 6, 2022 03:54
@jalavosus
Copy link
Contributor Author

Just had the thought that it might make sense to have the flag name check also check for default command's subcommands (should they exist), to make the whole thing real seamless.

@dearchap
Copy link
Contributor

dearchap commented May 7, 2022

I dont see a real use case for this. Are there any real world clis which make use of this functionality.

@meatballhat meatballhat added this to the Release 2.x milestone May 8, 2022
@jalavosus
Copy link
Contributor Author

I dont see a real use case for this. Are there any real world clis which make use of this functionality.

@dearchap

  • https://github.com/ahmetb/kubectx (defaults to listing all configured kubectl contexts)
  • yarn (defaults to yarn install)
  • python/python3 (defaults to loading a REPL if no arguments are specified)

Also, if the feature is able to be implemented (which, well, it clearly is) then why not implement it? It adds functional value to the package, and doesn't seem to be -- at least imo -- any sort of feature creep.

@meatballhat
Copy link
Member

@jalavosus Hello and sorry for the delay 😭 ! Are you up for resolving conflicts and getting tests happy? I agree with you regarding examples in the wild and I'd like to move forward with this work 👍🏼

@jalavosus jalavosus dismissed a stale review via 057d8a5 June 21, 2022 23:25
@jalavosus
Copy link
Contributor Author

@meatballhat Rebased onto main (hence the force push) and fixed the failing test (just needed to run gofmt)

@meatballhat
Copy link
Member

@jalavosus Thank you! Are you able to appease codecov, too? 😅

@jalavosus
Copy link
Contributor Author

Are you able to appease codecov, too?

I've been trying to, it does not like breaks.

Copy link
Member

@meatballhat meatballhat left a comment

Choose a reason for hiding this comment

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

Lovely! 🤩

@meatballhat meatballhat merged commit d29120f into urfave:main Jun 24, 2022
meatballhat added a commit that referenced this pull request Jun 24, 2022
@renovate renovate bot mentioned this pull request Jan 28, 2024
1 task
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.

Allow default command if one is not passed
3 participants