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

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

2 participants