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

Use Cobra CLI instead of docopt #116

Merged
merged 1 commit into from
Jul 5, 2019
Merged

Conversation

khos2ow
Copy link
Member

@khos2ow khos2ow commented Jun 29, 2019

Prerequisites

Put an x into the box(es) that apply:

  • This pull request fixes a bug.
  • This pull request adds a feature.
  • This pull request enhances existing functionality.
  • This pull request introduces breaking change.

For more information, see the Contributing Guide.

Description

This PR replaces docopt with Cobra CLI library. Using Cobra will give flexibility in terms of new subcommands or flags, and grouping them together.

Commands:

  • json to generate json formatted
  • pretty to generate colorized pretty output
  • markdown (md as alias) to generate Markdown format
    • table (tbl alias) to generate Markdown Table format
    • document (doc alias) to generate Markdown Document format
  • version to print the version of binary
  • completion to prepare SHELL completion
    • bash shell completion
    • zsh shell completion
  • help to show help message

Flags:

  • Global
    • --no-sort omit sorted rendering of inputs and outputs
    • --sort-inputs-by-required sort inputs by name and prints required inputs first
    • --with-aggregate-type-defaults print default values of aggregate types
    • --version to print the version of binary
    • -h, --help to show help message for binary and each subcommands
  • Only for markdown and its children commands:
    • --no-required omit "Required" column when generating Markdown
    • Note that this distinction to tie a flag to specific commands or subcommands wasn't possible previously

The migration was 99% backward compatible and without any functionality change except the default execution of binary shows help message and pretty print has been moved to its own standalone commands (Any comments or suggestion on this will be greatly appreciated)

Issues Resolved

List any existing issues this pull request resolves.

Checklist

Put an x into all boxes that apply:

Tests

  • I have added tests to cover my changes.
  • All tests pass when I run make test.

Documentation

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Code Style

  • My code follows the code style of this project.

@metmajer
Copy link
Member

@khos2ow docopt is less strict than cobra when it comes to options and argument positions. Hence, I expect that your change will break existing usages of terraform-docs. Since we do not have a use-case that requires the use of cobra, I see that the risks outweigh the value of this pull request.

I value your support for this project a lot. But note that the Contribution Guidelines ask for contributions to be discussed with the maintainers before filing a submission. This is meant to avoid work that is not in the best interest of the project. There are a lot of open issues that still seek for contributors and I'd be happy to support you if you want to submit a pull request for one of these.

@metmajer metmajer closed this Jun 30, 2019
@khos2ow
Copy link
Member Author

khos2ow commented Jun 30, 2019

Actually that's not correct, flag positioning in cobra is really flexible and can be provided before or after commands or subcommands, or even after the [PATH...]

Two other main benefits that Cobra gives but docopt doesn't, are:

  • auto completion for different SHELLs
  • tie flag to specific command (e.g. --no-reguired flag which only belongs to markdown command and its subsequent subcommands

And the bonus point (which might be useful eventually) is command aliases, e.g.

terraform-docs md tbl ./my-module

is completely valid.

@metmajer
Copy link
Member

@khos2ow Thanks for pointing this out. Shall we agree to reopen this after #62? Please head there for a recent update and call-to-action.

@khos2ow
Copy link
Member Author

khos2ow commented Jun 30, 2019

Sure, I like to be able to include this in the current release, and personally like it to be before #62 (if we're going to consider this PR at all) to prevent resolving merge conflict in the future. But I'm ok either way.

@metmajer
Copy link
Member

metmajer commented Jul 5, 2019

@khos2ow this is indeed a valuable and very well implemented addition to the code base and will help us better address some of the upcoming issues. Thanks for your patience!

@metmajer metmajer merged commit 5faf591 into terraform-docs:master Jul 5, 2019
@khos2ow khos2ow deleted the cobra-cmd branch July 5, 2019 14:19
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.

None yet

2 participants