-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
docs: improved --help for pin remote commands #7823
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small readability nits but looks great. Thank you!
Thanks 😃, this is great. Appreciate the comments from @jessicaschilling. Let me know when you're ready to merge. |
e5323f8
to
331922e
Compare
@aschmahmann applied improvements from @jessicaschilling, should be ready for merge 👍 |
|
||
Status of background pin requests can be inspected with the 'ls' command: | ||
|
||
$ ipfs pin remote ls --service=mysrv --cid=bafkqaaa --status=queued,pinning,pinned,failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We support --status=queued --status=pinning ...
via the StringsOption
option.
We currently have some code that allow each --status=ABC
that has commas in it to be flattened together, IMO this behavior is a little confusing and if we keep both then we should definitely document it.
@lidel why are we supporting the comma separated version is it for user ergonomics, compatibility with clients that refuse to send repeated HTTP query parameters or both?
If it's just for user ergonomics we may want to not tie ourselves to processing this server side and add this into the CLI processing for go-ipfs-cmds (e.g. https://github.com/ipfs/go-ipfs-cmds/blob/edc78ef36c1dadd4d1a87737e9a1727e59d36c45/cli/parse.go#L101).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aschmahmann
It is just for user ergonomics. I was playing with CLI and the alternative is pretty tedious.
I am considering adding --status=any
(https://github.com/ipfs/go-pinning-service-http-client/issues/7) to improve it even further.
My concern with adding it as a global feature of go-ipfs-cmds is that it will block use of ,
in values, it becomes a value separator. Here, its fine, as ,
won't be part of status
or cid
, but we don't know if its equally safe for other places: what if I want to pass multiple unix file paths, and one of them has ,
in file or dir name?
I suggest we keep it local to pin remote
commands to unblock 0.8.0.
We can refactor and figure out how to handle it globally in future releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about if we allowed passing an option into StringsOption with a delimiter. If the delimiter is defined (e.g. a comma, colon, etc) the it would be used, but otherwise you'd have to pass multiple options instead of using a delimiter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that would be 👌 – removes risk I described.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I'll poke around go-ipfs-cmds and see how annoying this is to implement. Thinking it should be a little painful but not too bad.
Note: we'll need docs describing this behavior and that the delimiter exists and only applies to the CLI since the HTTP API docs are generated from these comments too 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been implemented in go-ipfs-cmds and the dep bubbled into go-ipfs, this PR can be rebased on master to take advantage of it. #7863
This adds docs in form of longer description that is displayed when --help is passes on the CLI. While at it, unified some language to match fields in the config file and other places + included some tips and best practices which should improve onboarding experience.
Co-authored-by: Jessica Schilling <jessica@protocol.ai>
4e2d17f
to
0b6912e
Compare
@aschmahmann rebased and looks green, mind taking a final look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you @lidel and @jessicaschilling. All our examples here use the comma delimited syntax without specific callouts that:
- It is valid to do repeated option passing (i.e.
ipfs pin remote ls --service=mysrv --status=queued --status=pinned
- It is valid for the repeated options to be delimited (i.e.
ipfs pin remote ls --service=mysrv --status=queued,pinned --status=pinning,failed
) - The HTTP API doesn't work with any of this delimited business
We may want to do these in subsequent PRs (and perhaps just leave 3 to ipfs-inactive/http-api-docs#40).
For now I'm going to merge this though since it's much better than what we currently have.
This adds clarification suggested in #7823 (review)
This adds clarification suggested in ipfs#7823 (review)
This PR adds docs in form of a longer description that is displayed when
--help
is passed on the CLI.Those are pretty important, as CLI crowd often does not read docs online, and prefer to explore features this way.
While at it, unified some language to match fields in the config file and other places (thx @Gozala!) + included some tips and best practices which should improve onboarding experience.
@jessicaschilling this falls under our effort to improve onboarding: lmk if anything sounds awkward, or we could rephrase to make it even more intuitive.
cc @ipfs/wg-pinning-services
--help
previewsBelow are previews of the initial version of this PR:
Click to expand: ipfs pin remote service add --help
Click to expand: ipfs pin remote service ls --help
Click to expand: ipfs pin remote add --help
Click to expand: ipfs pin remote ls --help
Click to expand: ipfs pin remote rm --help