Skip to content
This repository has been archived by the owner on Mar 4, 2022. It is now read-only.

Start adding long help documentation strings #152

Merged
merged 7 commits into from
Jul 25, 2018
Merged

Start adding long help documentation strings #152

merged 7 commits into from
Jul 25, 2018

Conversation

bufdev
Copy link
Contributor

@bufdev bufdev commented Jul 16, 2018

This starts the work to add long documentation strings to commands. This does a few things:

  • Cleans up the combination of the Short and Long parameters by adding an extra newline.
  • Uses https://github.com/mitchellh/go-wordwrap to wrap the documentation to 80 characters. This repository is MIT-licensed, so it's acceptable to use, and this makes the documentation look much cleaner (I was slightly surprised to find out that Cobra doesn't have an option for this). Some lines have to be manually wrapped to work, namely things in lists (so the next line is indented), and commands (where I \- them out to separate lines when displayed). All the long documentation strings look proper when I try them out.
  • Copies over some of the documentation from README.md.

After this start, we can fill out the long documentation strings for other commands at will.

I'd recommend checking out this branch, doing make install, and then doing prototool help grpc, lint,compile,... to see how this looks.

@amckinney I know you have specific preferences for documentation, and it almost always just adds improvement, but my goal is to get this in as a start, and then iterate on the documentation. We can do one of a few things:

  1. Just iterate on it on this PR like normal (which is fine if that's what you want to do! No issues!).
  2. Land this, and have you do a new PR with any documentation changes you want.
  3. You do a chained PR with any documentation changes.

Or...

  1. You just push the changes you want directly to this branch/PR. This is probably the most efficient - instead of commenting the changes on the PR, just make the changes (which I'll almost for sure be fine with) and then we can land it.

(4) would probably be my preferred mechanism, but anything is fine, what do you think?

Fixes #146.

@codecov-io
Copy link

codecov-io commented Jul 16, 2018

Codecov Report

Merging #152 into dev will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #152      +/-   ##
==========================================
+ Coverage   67.68%   67.73%   +0.05%     
==========================================
  Files          73       73              
  Lines        3778     3778              
==========================================
+ Hits         2557     2559       +2     
+ Misses        882      881       -1     
+ Partials      339      338       -1
Impacted Files Coverage Δ
internal/cmd/templates.go 75.69% <100%> (+1.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f439cd7...54ca147. Read the comment docs.

@amckinney
Copy link
Contributor

Nice! Yea, let's take option (4) and add commits to this PR. Once we have all of the commands documented, we can land them all together.

@bufdev
Copy link
Contributor Author

bufdev commented Jul 16, 2018

Can you make this the next thing you work on regarding Prototool if possible? It'd be cool to close this out :-)

- add `wordWrapLength` const
- indent `create` file example
- reword init
Copy link
Contributor

@peats-bond peats-bond left a comment

Choose a reason for hiding this comment

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

👍 fixed some nits, but looks fine, also agree with strategy for this diff :)

@@ -161,6 +221,86 @@ var (
grpcCmdTemplate = &cmdTemplate{
Use: "grpc dirOrProtoFiles...",
Short: "Call a gRPC endpoint. Be sure to set required flags address, method, and either data or stdin.",
Long: `What this does behind the scenes:

- Compiles your Protobuf files with "protoc", generating a "FileDescriptorSet".
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this part may be unnecessary, describing a FileDescriptorSet. What if we simplified it as:

This command compiles your proto files with "protoc", converts JSON input to binary
and converts the result from binary to JSON. All these steps take on the order of 
milliseconds. For example, the overhead for a file with four dependencies is about
30ms, so there is little overhead for CLI calls to gRPC.

Commenting here to get both your thoughts, not sure if this is any clearer.

@bufdev
Copy link
Contributor Author

bufdev commented Jul 18, 2018

I'd like to merge this before doing #16 - can we do what we'd like and get this in?

Copy link
Contributor

@peats-bond peats-bond left a comment

Choose a reason for hiding this comment

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

I've made some changes, but I'm fine with it as is now.
Probably worth it for one of you two to do a quick skim.

It sounds like we want to merge this as is. I'm fine if we open up subsequent PRs to finish up the long help commands.

@bufdev
Copy link
Contributor Author

bufdev commented Jul 18, 2018

Why did you remove the description of what it does? I think users will want to know this @AlexAPB

@peats-bond
Copy link
Contributor

I got no response from my comment so I assumed you were fine with it :P

I was primarily trying to remove discussing a FileDescriptorSet since that is not a term we expose beyond the implementation, it just seemed like additional noise. I then found myself trying to simplifying it a bit more.

It was a minor readability change, we can revert if you'd like, this has my stamp.

@bufdev
Copy link
Contributor Author

bufdev commented Jul 20, 2018

Anything is fine. Ping @amckinney.

@bufdev
Copy link
Contributor Author

bufdev commented Jul 25, 2018

Merging per email with @AlexAPB.

@bufdev bufdev merged commit 7b50288 into dev Jul 25, 2018
@bufdev bufdev deleted the long-help-docs branch July 25, 2018 10:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants