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

Add synopsis autogenerator #2785

Merged
merged 7 commits into from
Jun 28, 2016
Merged

Add synopsis autogenerator #2785

merged 7 commits into from
Jun 28, 2016

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Jun 1, 2016

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
Copy link
Member

oooOooo, fancy! you should throw in a couple tests

@Kubuxu
Copy link
Member Author

Kubuxu commented Jun 1, 2016

I will.

@Kubuxu
Copy link
Member Author

Kubuxu commented Jun 1, 2016

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

@whyrusleeping
Copy link
Member

That works for me.

@Kubuxu Kubuxu removed the need_tests label Jun 1, 2016
@Kubuxu
Copy link
Member Author

Kubuxu commented Jun 1, 2016

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

@RichardLitt
Copy link
Member

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.

@RichardLitt RichardLitt added the topic/docs-ipfs Topic docs-ipfs label Jun 2, 2016
@Kubuxu
Copy link
Member Author

Kubuxu commented Jun 2, 2016

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
Copy link
Member

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

@Kubuxu
Copy link
Member Author

Kubuxu commented Jun 2, 2016

@Kubuxu Kubuxu force-pushed the feature/auto-synopsis branch 6 times, most recently from 8644a2a to e5010e2 Compare June 2, 2016 19:25
@Kubuxu
Copy link
Member Author

Kubuxu commented Jun 2, 2016

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
Copy link
Member Author

Kubuxu commented Jun 2, 2016

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
Copy link
Member

@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 Kubuxu force-pushed the feature/auto-synopsis branch 2 times, most recently from 1df32ed to c509973 Compare June 2, 2016 20:34
@Kubuxu
Copy link
Member Author

Kubuxu commented Jun 2, 2016

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

@Kubuxu
Copy link
Member Author

Kubuxu commented Jun 3, 2016

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

@RichardLitt
Copy link
Member

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
Copy link
Member Author

Kubuxu commented Jun 3, 2016

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

@RichardLitt
Copy link
Member

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>...]

@whyrusleeping
Copy link
Member

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 Kubuxu mentioned this pull request Jun 17, 2016
6 tasks
@Kubuxu Kubuxu force-pushed the feature/auto-synopsis branch 2 times, most recently from 002930d to 61b832b Compare June 17, 2016 22:20
@Kubuxu Kubuxu changed the title Add synopsis autogenerator and no- prefix bool options Add synopsis autogenerator Jun 17, 2016
@Kubuxu
Copy link
Member Author

Kubuxu commented Jun 18, 2016

@whyrusleeping removed the no- prefixing.

@Kubuxu Kubuxu added this to the Ipfs 0.4.3 milestone Jun 21, 2016
@whyrusleeping
Copy link
Member

@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
Copy link
Member

(the few commands i ran that didnt hang looked great 👍 )

@whyrusleeping
Copy link
Member

@Kubuxu could you rebase this one?

Generates synopsis automagically

License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
As synopsis is only one line long it is overkill

License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Copy link
Member Author

Kubuxu commented Jun 28, 2016

Rebased.

@whyrusleeping
Copy link
Member

LGTM!

@whyrusleeping whyrusleeping merged commit 7fa88fd into master Jun 28, 2016
@whyrusleeping whyrusleeping deleted the feature/auto-synopsis branch June 28, 2016 19:05
@RichardLitt
Copy link
Member

YASSSSSSSS

@jbenet
Copy link
Member

jbenet commented Aug 27, 2016

  • This is great
  • But a problem: one of the goals for synopsis was to give users a very clear example of the top 1-3 commands they would be running, like the simple usage: that you see in many commands. With a full synopsis, that's not there anymore, so we need to either:
    • prioritize examples and suck it up that they dont show on short -h
    • allow a different usage: thing to exist.
  • Compare git merge -h (the usage at the top) and git merge --help (the full synopsis).

--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
Copy link
Member

jbenet commented Aug 27, 2016

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
Copy link
Member

jbenet commented Aug 27, 2016

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

@jbenet jbenet mentioned this pull request Aug 28, 2016
58 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review topic/docs-ipfs Topic docs-ipfs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants