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

doc: Documents the offset/length arguments to ipfs.files.cat and friends #242

Merged

Conversation

achingbrain
Copy link
Collaborator

@ghost ghost assigned achingbrain Mar 21, 2018
@ghost ghost added the in progress label Mar 21, 2018
SPEC/FILES.md Outdated
@@ -161,9 +161,9 @@ pull(

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

##### `JavaScript` - ipfs.files.cat(ipfsPath, [callback])
##### `JavaScript` - ipfs.files.cat(ipfsPath, [begin], [end], [callback])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why args like this? Let's add options and have that begin and end there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair enough, will change. I've noticed that we have a ipfs.files.read function that accepts offset and count - it might be better to use that instead of begin and end to be consistent. What do you think?

Also, I can't find an implementation of that in js-ipfs but it looks like it could just call through to ipfs.files.cat?

Copy link
Contributor

Choose a reason for hiding this comment

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

offset and count

👍

ipfs.files.read

This is a MFS specific operation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

offset and count

👍

Done

SPEC/FILES.md Outdated
@@ -197,9 +199,9 @@ A great source of [examples][] can be found in the tests for this API.

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

##### `JavaScript` - ipfs.files.catReadableStream(ipfsPath) -> [Readable Stream][rs]
##### `JavaScript` - ipfs.files.catReadableStream(ipfsPath, [begin], [end]) -> [Readable Stream][rs]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

SPEC/FILES.md Outdated
@@ -225,9 +230,9 @@ A great source of [examples][] can be found in the tests for this API.

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

##### `JavaScript` - ipfs.files.catPullStream(ipfsPath) -> [Pull Stream][rs]
##### `JavaScript` - ipfs.files.catPullStream(ipfsPath, [begin], [end]) -> [Pull Stream][rs]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

@achingbrain achingbrain force-pushed the add-files-cat-slice-arguments branch from f21ef75 to 7fd182e Compare March 29, 2018 09:06
Copy link
Contributor

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

@achingbrain This PR is missing tests. The tests need to pass in both js-ipfs and js-ipfs-api. I do understand that go-ipfs doesn't support offset reads in .cat yet, but that can be shimmed in js-ipfs-api until go-ipfs ships that feature.

@richardschneider
Copy link
Contributor

The go documentation uses offset and length. Should we use these? offset and begin are the same except for naming; whereas length and end have different semantics.

@achingbrain
Copy link
Collaborator Author

@diasdavid I was going to move some of the tests from js-ipfs-unixfs-engine to this module as a follow up in order to not break anything if the modules were released one at a time. Happy to do it all in one go if we're manually batching up module releases.

@richardschneider Yes, we'd discussed that on the outdated changes that Github has hidden above. The currently go-only ipfs.files.read function uses offset and count instead of offset and length however which reads a little weirdly to me but agreed consistency is more important.

@achingbrain achingbrain force-pushed the add-files-cat-slice-arguments branch from 7fd182e to ffa4f76 Compare April 2, 2018 06:00
achingbrain added a commit to ipfs-inactive/js-ipfs-unixfs-engine that referenced this pull request Apr 9, 2018
achingbrain added a commit to ipfs-inactive/js-ipfs-unixfs-engine that referenced this pull request Apr 9, 2018
achingbrain added a commit to ipfs-inactive/js-ipfs-unixfs-engine that referenced this pull request Apr 9, 2018
achingbrain added a commit to ipfs-inactive/js-ipfs-unixfs-engine that referenced this pull request Apr 10, 2018
achingbrain added a commit to ipfs-inactive/js-ipfs-unixfs-engine that referenced this pull request Apr 10, 2018
@achingbrain achingbrain force-pushed the add-files-cat-slice-arguments branch from ffa4f76 to 4b39a4f Compare April 10, 2018 16:52
@achingbrain achingbrain changed the title doc: Documents the begin/end arguments to ipfs.files.cat and friends doc: Documents the offset/length arguments to ipfs.files.cat and friends Apr 10, 2018
@achingbrain
Copy link
Collaborator Author

After a straw poll in the js-dev catchup it was decided that offset/length is preferable to offset/count.

@diasdavid I've added some tests for stream slices that should only run for js-ipfs.

There's quite a lot of scope for test duplication across all these modules, what's the general approach? In depth tests here and 'smoke test' style tests in dependencies?

@daviddias daviddias merged commit 4d8a744 into ipfs-inactive:master Apr 14, 2018
@ghost ghost removed the in progress label Apr 14, 2018
daviddias pushed a commit to ipfs-inactive/js-ipfs-unixfs-engine that referenced this pull request Apr 23, 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.

3 participants