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

@jalavosus jalavosus commented May 6, 2022

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
@jalavosus
Copy link
Contributor Author

jalavosus commented May 6, 2022

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

jalavosus commented May 8, 2022

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

meatballhat commented Jun 18, 2022

@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 Jun 21, 2022
@jalavosus jalavosus force-pushed the feature/default-command branch from 057d8a5 to 1dfa982 Compare Jun 21, 2022
@jalavosus
Copy link
Contributor Author

jalavosus commented Jun 21, 2022

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

@meatballhat
Copy link
Member

meatballhat commented Jun 22, 2022

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

@jalavosus
Copy link
Contributor Author

jalavosus commented Jun 23, 2022

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

Lovely! 🤩

@meatballhat meatballhat merged commit d29120f into urfave:main Jun 24, 2022
11 of 13 checks passed
meatballhat added a commit that referenced this issue Jun 24, 2022
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.

3 participants