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

CLI: discoverability and consistency #2542

Merged
merged 36 commits into from
Apr 28, 2016
Merged

Conversation

hackergrrl
Copy link
Contributor

CLI: Discoverability & Consistency

Hi everyone!

I've made some changes to the ipfs CLI tool's output. These aren't major changes, but they're maybe enough to warrant talking about: nobody likes their CLI tool's output suddenly changing from underneath them. These changes are not major, but do represent a change.

That's very fair, so I wanted to share my design approach and goals to better justify the changes I've made here. I'm a fan of esr's unix rules, and so my commits here reflect that (In particular, discoverability). The changes are broken down by the high level goal they fit under below.

I expect this to take several rounds to complete -- CLI output is not a standardized thing and is also ripe for bikeshedding. Let's try to focus on the goals below and not quibble on style too much.

Goals

Discoverability

New users start using ipfs every day. CLI tools don't expose their affordances the same way GUI apps do (no toolbar along the top), so enabling users to discover the functionality of the tool is paramount, via both showing subcommands and showing short-help where ever possible.

Subcommands

  • running a command without arugments should show its subcommands
    • ipfs block
    • ipfs file
    • ipfs pin
    • ipfs repo
    • ipfs dht
    • ipfs diag
    • 'ipfs bootstrap' should show help, not alias 'ipfs bootstrap list'
  • removes redundant cases where subcommands are manually listed in SYNOPSIS
    (in favour of using autogen everywhere)

Short-Help

  • a parse error on invoking a command should show its short-help
    • ipfs config
    • ipfs add
    • ipfs cat
    • ipfs get
    • ipfs ls
    • ipfs refs
    • ipfs resolve
    • ipfs dns
    • ipfs ping
  • removed ipfs file from the command listing (confirmed with
    @whyrusleeping that this is kinda crufty)
  • split 'ipfs daemon' shorthelp into short- and long- sections

Formatting

Terminals come in many shapes and sizes. 80x25 is the default terminal size on most older curses-based displays, and is the yard stick that many terminal programs are aimed to display reasonably on. Where possible, horizontal wrapping at ~80 characters has been done, and needless vertical whitespace is minimized. This also tackles consistency of output across the various command outputs.

  • Terminal formatting / sizing
    • reduce horizontal indent on help docs
    • reduce vertical whitespace on help docs (ipfs in particular)
  • Consistency
    • main 'ipfs' cmd doesn't use colons on subheadings; let's not use them for
      subcommands either
  • move long tagline details into DESCRIPTION
  • split long ShortDescriptions into Short+Long Description text

@hackergrrl
Copy link
Contributor Author

@@ -20,7 +20,7 @@ const (

whitespace = "\r\n\t "

indentStr = " "
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be more of enforcing a personal opinion or habit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No -- this falls under "Terminal formatting / sizing: reduce horizontal indent on help docs".

Let's reduce the tool's horizontal footprint so 80 char terminals don't experience weird line wrap. (Fully dynamic smart wrapping would be even cooler, but that's a bit bigger scope than 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 don't think that the commands lib should change the formatting of the helptext at all, otherwise we might try to format things a certain way only to have the code screw things up.

A better option I think is to have a commands_test.go file (or similar) that goes through each command and ensures their helptext looks correct (tabs vs spaces, etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed that this should be a concern of the library user and not the core commands lib. I can file an issue for that -- it seems like a bigger job than what's in scope for this PR.

@hackergrrl
Copy link
Contributor Author

Thanks for taking a look and feedback, @diasdavid!

@whyrusleeping
Copy link
Member

should we just remove the synopsis field entirely?

@hackergrrl
Copy link
Contributor Author

Yeah, we kind of use the Usage and Tagline to mean Synopsis throughout. I don't think it's a bad thing to expose to the CLI API's users though, if they choose to use Usage/Tagline/Synopsis differently.

@RichardLitt
Copy link
Member

I <3 you, Noffle. This is great work.

Send pings to a peer using the routing system to discover its address
`,
Tagline: "Send echo request packets to IPFS hosts.",
Synopsis: "Send pings to a peer using the routing system to discover its address",
Copy link
Member

Choose a reason for hiding this comment

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

Needs .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I also went through the other commands to check for consistent perioding.

@hackergrrl
Copy link
Contributor Author

All green!

@@ -30,11 +30,9 @@ const (

var AddCmd = &cmds.Command{
Helptext: cmds.HelpText{
Tagline: "Add a file to ipfs.",
Tagline: "Add a file or directory hierarchy to ipfs.",
Copy link
Member

Choose a reason for hiding this comment

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

I'd drop hierarchy from this, Add a file or directory to ipfs is short sweet and to the point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@whyrusleeping
Copy link
Member

t0235 is failing for some reason

@hackergrrl
Copy link
Contributor Author

hackergrrl commented Apr 25, 2016

I was trying out developing on my NixOS laptop and I think it's using a different netcat (gnu vs bsd), so I chose flags that didn't exist here. Reverted that work in favour of figuring it out another day.

},

Run: bootstrapListCmd.Run,
Copy link
Member

Choose a reason for hiding this comment

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

i'm not so sure i'm onboard with this change still.. I like commands being able to have an effect when run without arguments.

Copy link
Contributor Author

@hackergrrl hackergrrl Apr 28, 2016

Choose a reason for hiding this comment

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

If only we had aliases!

$ ipfs alias set bootstrap = bootstrap list

I can revert this though, if that's going to block the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@whyrusleeping
Copy link
Member

@noffle now this needs to be rebased.. theres a bunch of commits here that don't seem right at all

License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
@hackergrrl
Copy link
Contributor Author

Fixed -- not sure what happened there.

@@ -29,14 +29,6 @@ test_bootstrap_list_cmd() {
echo "$BP" >>list_expected
done

test_expect_success "'ipfs bootstrap' succeeds" '
Copy link
Member

Choose a reason for hiding this comment

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

we should put these back in now that the functionality has been restored.

hackergrrl and others added 21 commits April 28, 2016 22:47
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
License: MIT
Signed-off-by: Stephen Whitmore <noffle@ipfs.io>
@hackergrrl
Copy link
Contributor Author

Pulled out my bootstrap list commits.

@whyrusleeping
Copy link
Member

sweet, LGTM. Thanks!

@whyrusleeping whyrusleeping merged commit 0a45ada into ipfs:master Apr 28, 2016
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.

4 participants