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

object patch rm-link: change arg from 'link' to 'name' #5638

Merged
merged 1 commit into from
Oct 26, 2018

Conversation

rcarver
Copy link
Contributor

@rcarver rcarver commented Oct 24, 2018

This improves the docs and semantics of rm-link to be consistent with add-link, clarifying that you should specify the link name as the thing to remove. I found the current docs, <link> ambiguous as to whether it was the name or the hash.

New output for Ipfs object patch --help

USAGE
  ipfs object patch - Create a new merkledag object based on an existing one.

SYNOPSIS
  ipfs object patch

DESCRIPTION

  'ipfs object patch <root> <cmd> <args>' is a plumbing command used to
  build custom DAG objects. It mutates objects, creating new objects as a
  result. This is the Merkle-DAG version of modifying an object.

SUBCOMMANDS
  ipfs object patch add-link <root> <name> <ref> - Add a link to a given object.
  ipfs object patch append-data <root> <data>    - Append data to the data segment of a dag node.
  ipfs object patch rm-link <root> <name>        - Remove a link from a given object.
  ipfs object patch set-data <root> <data>       - Set the data field of an IPFS object.

  Use 'ipfs object patch <subcmd> --help' for more information about each command.

New output for ipfs object patch rm-link --help

USAGE
  ipfs object patch rm-link <root> <name> - Remove a link from a given object.

SYNOPSIS
  ipfs object patch rm-link [--] <root> <name>

ARGUMENTS

  <root> - The hash of the node to modify.
  <name> - Name of the link to remove.

DESCRIPTION

  Remove a Merkle-link from the given object and return the hash of the result.

This improves the semantics of rm-link to be consistent with add-link,
clarifying that you should specify the link name as the thing to remove.

License: MIT
Signed-off-by: Ryan Carver <ryan@ryancarver.com>
@rcarver rcarver requested a review from Kubuxu as a code owner October 24, 2018 17:15
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

The dock change looks good but the arg change will break a public API. We do sometimes change the API but prefer not to unless there's a strong motivation (i.e., an actual bug).

`,
},
Arguments: []cmdkit.Argument{
cmdkit.StringArg("root", true, false, "The hash of the node to modify."),
cmdkit.StringArg("link", true, false, "Name of the link to remove."),
cmdkit.StringArg("name", true, false, "Name of the link to remove."),
Copy link
Member

Choose a reason for hiding this comment

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

While I prefer this new flat, it breaks the API and I'm not convinced it's really worth the potential issues.

Copy link
Contributor Author

@rcarver rcarver Oct 24, 2018

Choose a reason for hiding this comment

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

@Stebalien oh interesting, I assumed the name of positional args wasn't part of a public API. Out of curiosity, where does it cause problems? Is there a test for it?

Copy link
Member

Choose a reason for hiding this comment

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

Oh. Sorry, I completely missed the fact that this was positional. This looks fine.

@Stebalien Stebalien requested a review from kevina October 24, 2018 19:26
@Stebalien Stebalien merged commit 90d8cff into ipfs:master Oct 26, 2018
@rcarver rcarver deleted the fix/cmds/rm-link branch October 26, 2018 19:48
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants