ipfs / kubo

An IPFS implementation in Go
https://docs.ipfs.tech/how-to/command-line-quick-start/
Other
15.83k stars 2.96k forks source link

Add synopsis autogenerator #2785

Closed Kubuxu closed 7 years ago

Kubuxu commented 7 years ago

Generates synopsis automagically

Few examples:

ipfs add [--recursive | -r] [--quiet | -q] [--silent] [--[no-]progress] [--trickle | -t] [--only-hash | -n] [--wrap-with-directory | -w] [--hidden | -H] [--chunker=<chunker> | -s] [--[no-]pin] [--] <path>... ipfs pin ls [--type=<type> | -t] [--quiet | -q] [--] [<ipfs-path>...]

License: MIT Signed-off-by: Jakub Sztandera kubuxu@protonmail.ch

whyrusleeping commented 7 years ago

oooOooo, fancy! you should throw in a couple tests

Kubuxu commented 7 years ago

I will.

Kubuxu commented 7 years ago

@whyrusleeping tell me if that is enough, if it is not then I will work more on it.

whyrusleeping commented 7 years ago

That works for me.

Kubuxu commented 7 years ago

@RichardLitt I would like to hear your opinion and also possible improvements if you see any.

RichardLitt commented 7 years ago

This looks great to me!

I can't think of any improvements, at the moment -- would be good to run this and add the output for each command to this PR? That way we can see how it works and upload the changes now, as opposed to after merging.

Kubuxu commented 7 years ago

If we remove synopsis from command now it will be replaced with the generated one. We could remove all of them from code, generate all help texts and go through them improving them (with SynopsisOptionsValues map). How does it sound?

RichardLitt commented 7 years ago

Sounds good to me. I think that makes more sense than merging this before doing that.

Kubuxu commented 7 years ago

All synopsis in existence: https://gist.github.com/Kubuxu/09b0ef346b469001ca51f6e345d79a33

Kubuxu commented 7 years ago

Synopsis lines can get quite long, should we do anything about it? Example:

ipfs add [--recursive=<recursive> | -r] [--quiet=<quiet> | -q] [--silent=<silent>] [--progress=<progress> | -p] [--trickle=<trickle> | -t] [--only-hash=<only-hash> | -n] [--wrap-with-directory=<wrap-with-directory> | -w] [--hidden=<hidden> | -H] [--chunker=<chunker> | -s] [--pin=<pin>] [--] <path>...
Kubuxu commented 7 years ago

Now with synopsis generator also would be great to go through codebase and make longer options definitions the primary ones (create as first instead of e).

Or I could also check which is the longest and use that one in the generator. What do you think?

whyrusleeping commented 7 years ago

@Kubuxu i think we should standardize on the options being specified a certain way. Making the 'first' one the primary, and the second the alternate

Kubuxu commented 7 years ago

WHOO, finally tests are green. Will work on moving those parameters around now.

Kubuxu commented 7 years ago

@RichardLitt re: gist: I could make the description in case of bool options a bool. Or maybe some way to show default value.

RichardLitt commented 7 years ago

I could make the description in case of bool options a bool. Or maybe some way to show default value.

That should work. The default value should always be specified by .Default(false) for bools - we can see if there's any issue with that once generated? Happy to go through each and nitpick.

Kubuxu commented 7 years ago

Only problem I have is how to show it? [--progress=??? | -p ]

RichardLitt commented 7 years ago

How about just [--progress | -p]?

Like this:

10:28 ~/src/community (feature/add-repo-links) 🐕  man git-add

GIT-ADD(1)                        Git Manual                        GIT-ADD(1)

NAME
       git-add - Add file contents to the index

SYNOPSIS
       git add [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]
                 [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]
                 [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing]
                 [--] [<pathspec>...]
Kubuxu commented 7 years ago

Then we should change options like: --pin to --no-pin or --progress to --no-progress and so on. I had no idea for a long time that you could do --pin=false.

RichardLitt commented 7 years ago

I agree with that. Knowing you can set it as --pin=false is not intuitive, I think.

Kubuxu commented 7 years ago

Example synopsis for command that has some parameter set to default true: ipfs add [--recursive | -r] [--quiet | -q] [--silent] [--[no-]progress] [--trickle | -t] [--only-hash | -n] [--wrap-with-directory | -w] [--hidden | -H] [--chunker=<chunker> | -s] [--[no-]pin] [--] <path>...

Kubuxu commented 7 years ago

I've also updated the gist with all synopsis generated: https://gist.github.com/Kubuxu/09b0ef346b469001ca51f6e345d79a33

EDIT: The parsing is temporarily broken.

chriscool commented 7 years ago

I wonder what will happen if we have for example 2 options that are incompatible with each other.

Kubuxu commented 7 years ago

We have, ipfs files stat has --format=<format> --hash --size which are incompatible. Or do you mean something else?

RichardLitt commented 7 years ago

I think that's fine. This is the synopsis, not example usage maximally.

chriscool commented 7 years ago

Yeah, maybe it is ok.

In case of incompatible options git shows something like:

git cat-file (-t [--allow-unknown-type]| -s [--allow-unknown-type]| -e | -p | <type> | --textconv ) <object>

or something like:

       git config [<file-option>] --unset name [value_regex]
       git config [<file-option>] --unset-all name [value_regex]
       git config [<file-option>] --rename-section old_name new_name
...
Kubuxu commented 7 years ago

go-ipfs command engine doesn't have currently incompatible options notation but it would be also something nice to have. I also have nice idea how it would work. Maybe later.

Kubuxu commented 7 years ago

Anyone has an idea for a command with boolean option that is true by default and can be tested easily?

whyrusleeping commented 7 years ago

@Kubuxu ipfs add has pin default to true

Kubuxu commented 7 years ago

Oh, and I can do ipfs pin ls great.

Kubuxu commented 7 years ago

I think it is ready for review. @RichardLitt and @whyrusleeping would you do the honours?

whyrusleeping commented 7 years ago

the synopsis generation LGTM. I'm still not super sure what i think of the --no- prefixing... Lets wait for a little more input on that on the issue about this before merging.

Kubuxu commented 7 years ago

@whyrusleeping removed the no- prefixing.

whyrusleeping commented 7 years ago

@Kubuxu i pulled this down to try it out, but --help is hanging on a bunch of commands due to the stdin thing. Lets get that fixed first and then i'll merge this in

whyrusleeping commented 7 years ago

(the few commands i ran that didnt hang looked great :+1: )

whyrusleeping commented 7 years ago

@Kubuxu could you rebase this one?

Kubuxu commented 7 years ago

Rebased.

whyrusleeping commented 7 years ago

LGTM!

RichardLitt commented 7 years ago

YASSSSSSSS

jbenet commented 7 years ago

--no- prefix

Never, please never use --no- prefix. It's written down elsewhere, but the gist is that double negatives absolutely suck, and are very, very confusing for people. It's also confusing having to remember which option (positive or negative) exists. It's much cleaner to establish an invariant that always options are positive, and if they default to true, you can turn them off with --thing=false.

jbenet commented 7 years ago

I think we need our short usage back, like git merge -h:

> git merge -h
usage: git merge [<options>] [<commit>...]
   or: git merge [<options>] <msg> HEAD <commit>
   or: git merge --abort

    -n                    do not show a diffstat at the end of the merge
    --stat                show a diffstat at the end of the merge
    --summary             (synonym to --stat)
    --log[=<n>]           add (at most <n>) entries from shortlog to merge commit message
...
jbenet commented 7 years ago

this could be explicitly chosen per command, and not auto generated, it's ok.