Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

docs: improve command usage documentation #709

Merged
merged 5 commits into from
Mar 25, 2024
Merged

docs: improve command usage documentation #709

merged 5 commits into from
Mar 25, 2024

Conversation

phm07
Copy link
Contributor

@phm07 phm07 commented Mar 7, 2024

This PR makes command usage documentation consistent by enforcing docopt rules and also defining and documenting other additional conventions.

Fixes #708

@phm07 phm07 self-assigned this Mar 7, 2024
@phm07 phm07 requested a review from a team as a code owner March 7, 2024 14:21
Copy link

codecov bot commented Mar 7, 2024

Codecov Report

Attention: Patch coverage is 91.01124% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 58.77%. Comparing base (4712950) to head (9457dc5).
Report is 2 commits behind head on main.

Files Patch % Lines
internal/cmd/base/delete.go 50.00% 1 Missing and 1 partial ⚠️
internal/cmd/base/labels.go 0.00% 2 Missing ⚠️
internal/cmd/base/set_rdns.go 0.00% 1 Missing ⚠️
internal/cmd/base/update.go 0.00% 1 Missing ⚠️
internal/cmd/server/enable_protection.go 0.00% 1 Missing ⚠️
internal/cmd/server/ssh.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #709      +/-   ##
==========================================
- Coverage   58.78%   58.77%   -0.02%     
==========================================
  Files         179      179              
  Lines        6536     6539       +3     
==========================================
+ Hits         3842     3843       +1     
- Misses       2087     2088       +1     
- Partials      607      608       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CONTRIBUTING.md Outdated
Command usage descriptions (namely `cobra.Command.Use`) should follow the [docopt](http://docopt.org/) syntax.

Additionally:
- Optional arguments should be the first in the usage string.
Copy link
Member

@jooola jooola Mar 7, 2024

Choose a reason for hiding this comment

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

Not sure to understand this, the main difference between multiple arguments is their position, so moving them around would be a breaking change.

EDIT: are you referring to flags/options ? For me flags/options != arguments.

Was weird that you wrote optional arguments, as they are 'required' by definition.

But yeah, I'd rather have the arguments first, and the options after. What does docopts say?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, you're right, I meant flags there. Docopt doesn't say anything about argument/flag order. Having optional flags first, then arguments and then required flags made the most sense to me because then the ordering would be most specific to least specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About the ordering, this is what the CLI Guidelines have to say: https://clig.dev/#help
They suggest putting the [options] first as well (like jq and git do it too for example). But generally they put all flags first (even required ones) and the positional arguments last, while we do optional flags < positional args < required flags. Maybe we can move the positional args all the way to the back like jq does.
On the other hand, if you look here for example, positional args would be easily overlooked if they are placed last. What's your opinion on this?

Copy link
Member

Choose a reason for hiding this comment

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

I would always try to have the required and optional flags next to each other, having the arguments in the middle seem weird.

Whether we put the arguments at the beginning or the end, I think I prefer at the beginning. When I write a command, I prefer to go in order of importance <resource type> <actions> <resource id> <resource fields (e.g. labels)>. Having the flags before also seem weird.

Copy link
Member

Choose a reason for hiding this comment

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

I do not have a strong opinion on this.

While using the CLI I use the same mental model as @jooola and also put <args> <req flags> <optional flags>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reordered the usage strings in 4ffc625. It is now in line with the CLI guideline suggestions

Copy link
Member

Choose a reason for hiding this comment

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

It looks like you only committed some of the changes, there are still many files with old order, for example internal/cmd/loadbalancer/update_service.go or internal/cmd/firewall/remove_from_resource.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I totally forgot to commit the changes, I did not just not see them ^^
Fixed in 9457dc5

@phm07 phm07 force-pushed the docopt branch 2 times, most recently from 68d8696 to e110d8d Compare March 7, 2024 16:01
This PR makes command usage documentation consistent by enforcing [docopt](http://docopt.org/) rules and also defining and documenting other additional conventions.
Copy link
Member

@apricote apricote left a comment

Choose a reason for hiding this comment

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

Very nice!

phm07 and others added 3 commits March 12, 2024 11:15
@apricote apricote merged commit ab6ef0e into main Mar 25, 2024
4 checks passed
@apricote apricote deleted the docopt branch March 25, 2024 06:58
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve commands help messages
3 participants