-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
Support root commands that doesn't implement call
#135
base: main
Are you sure you want to change the base?
Conversation
@@ -438,6 +438,18 @@ def call(**params) | |||
end | |||
end | |||
|
|||
class Namespace < Dry::CLI::Command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't bizarre to have a "command" that the uniq usage is to print top level usage? I am wondering if it should be a better idea to move the usage of top class as specific dsl. Like suggested here #96 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is. I tried to implement the suggestion from #96, but I couldn't figure out how to do it. Maybe we can create the Dry::CLI::Namespace
class, so It will look less strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gustavothecoder I like the idea of this being a specialised Namespace
class. That way, when the class is used, it will be clearer that its purpose is to provide information about a grouping of subcommands only, and not its own distinct command behaviour.
I prefer this help text residing in a concrete class instead of being part of the top-level command registration API, since that feels more consistent with how the rest of the CLI construction is done: a class for each thing.
If you're still interested in working on this PR, I'd love it if you could make this adjustment! Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timriley Done! I think it looks better using the Namespace
class.
This is a great proposal. I would love to be able to use this in our CLI. |
6163287
to
3b16959
Compare
Resolve #96
I would like to add descriptions to my root commands without implementing a proper command, like a namespace.
Example
Command
Result