Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

docs: remove array arguments and document missing options #277

Merged
merged 2 commits into from
Jun 13, 2018

Conversation

achingbrain
Copy link
Collaborator

Further to the conversation on #262 I've swept the mfs api and made what I hope are improvements to consistency and added flags and options where I've found them undocumented in the codebase or where they were otherwise missing.

Most of this has been implemented in ipfs/js-ipfs-mfs.

@ghost ghost assigned achingbrain May 18, 2018
@ghost ghost added the in progress label May 18, 2018
@achingbrain achingbrain requested review from alanshaw and daviddias May 18, 2018 18:31
SPEC/FILES.md Outdated

If no `callback` is passed, a promise is returned.

**Example:**

```JavaScript
ipfs.files.cp(['/src-file', '/dst-file'], (err) => {
ipfs.files.cp('/src-file', '/dst-file', (err) => {s
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a trailing "s" here

SPEC/FILES.md Outdated
- `from` is the path of the source file to copy.
- `to` is the path of the destination file to copy to.
- `from` is the path(s) of the source to copy. It might be:
- An existing MFS path (e.g. `/my-dir/my-file.txt`)
Copy link
Contributor

Choose a reason for hiding this comment

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

A note or example to clarify that from can be a file or a directory would be good here.

#### `files.cp`

> Copy files.

##### `Go` **WIP**

##### `JavaScript` - ipfs.files.cp([from, to], [callback])
##### `JavaScript` - ipfs.files.cp(...from, to, [options], [callback])
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 nice API

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't claim it, it's stolen from cp..

SPEC/FILES.md Outdated
- `hash` is a Boolean value to return only the hash (default: false)
- `size` is a Boolean value to return only the size (default: false)
- `withLocal` is a Boolean value to compute the amount of the dag that is local, and if possible the total size (default: false)
- `callback` is an optional function with the signature `function (error, stats) {}`, where `error` may be an Error that occured if the operation was not successful and `stat` is an Object with the following keys:
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of the result parameter changes! Needs an add "s" to stat or remove an "s" from stats in function (error, stats) {}.

SPEC/FILES.md Outdated
@@ -738,19 +765,20 @@ ipfs.files.rm('/my/beautiful/directory', { recursive: true }, (err) => {

Where:

- `path` is the path of the file to read.
- `path` is the path of the file to read and must point to a file
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding "not a directory" to the end of that sentence would make it clearer - I think that's what you mean? (also for readPullStream and readReadableStream please!)

SPEC/FILES.md Outdated
}
})

ipfs.files.cp('/src-dir', '/dst-dir', (err) => {s
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be helpful for the reader if the difference between this example and the one above was highlighted e.g. // To copy a directory - I thought it was a duplication when I first glanced at it 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

For copying a single file, the destination must not exist, but for copying a single directory or multiple files/directories the destination needs to exist and be a directory (or the parents option needs to be passed to create the destination directory and any parents).

If that's right(?), it's really useful info that should be mentioned alongside "If there are multiple sources..." above (or something to that effect!).

Copy link
Contributor

Choose a reason for hiding this comment

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

These changes will need to be applied to mv also as it has got a similar API.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's also that trailing "s" again.

@achingbrain
Copy link
Collaborator Author

@alanshaw I've updated the docs around cp/mv - hopefully they're a bit clearer now.

@achingbrain achingbrain merged commit 510516a into master Jun 13, 2018
@ghost ghost removed the in progress label Jun 13, 2018
@achingbrain achingbrain deleted the files-spec-update branch June 13, 2018 10:13
achingbrain added a commit to ipfs-inactive/js-ipfs-http-client that referenced this pull request Jun 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants