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

fix: files ls inconsisntency #776

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

hacdias
Copy link
Contributor

@hacdias hacdias commented May 23, 2018

It is stated on interface-ipfs-core that files.ls type's must be strings (file or directory).

Tests: ipfs-inactive/interface-js-ipfs-core#282

@@ -8,7 +8,7 @@ const transform = function (res, callback) {
callback(null, entries.map((entry) => {
return {
name: entry.Name,
type: entry.Type,
type: (entry.Type === 0) ? 'file' : 'directory',
Copy link
Contributor

Choose a reason for hiding this comment

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

The unixfs type for file is 2, and directory is 1.

Sources:
[0] https://github.com/ipfs/js-ipfs-unixfs/blob/master/src/unixfs.proto.js#L6,L7
[1] https://github.com/ipfs/js-ipfs/blob/master/src/http/api/resources/files.js#L323,L332

Also per source [1], it appears that js-ipfs-unixfs-engine uses dir[2] instead of directory. However, js-ipfs-unixfs defines the string type as directory[3].

[2] https://github.com/ipfs/js-ipfs-unixfs-engine/blob/master/src/exporter/dir-flat.js#L32
[3] https://github.com/ipfs/js-ipfs-unixfs/blob/master/src/index.js#L11

I'm unsure if this is going to cause a conflict immediately though, but it looks like js-ipfs-unixfs-engine should be updated as well at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unixfs type for file is 2, and directory is 1.

That's strange, because I'm getting 0s for files and 1s for directories... Hmmm, well, I'll take a look at this later then. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

What are the numbers in go-ipfs?

Copy link

Choose a reason for hiding this comment

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

They are here: https://github.com/ipfs/go-ipfs/blob/v0.4.15/mfs/system.go#L39-L42 (I ran into this separately.) Talking with @whyrusleeping on IRC about it, because a) confusing and b) not well documented. I'd also expected them to match UnixFS.

@daviddias daviddias requested review from alanshaw and removed request for olizilla and daviddias July 17, 2018 06:52
@hacdias
Copy link
Contributor Author

hacdias commented Aug 4, 2018

@alanshaw what do you think about this?

@hacdias
Copy link
Contributor Author

hacdias commented Feb 26, 2019

@alanshaw what's the state of this PR? The same for ipfs-inactive/interface-js-ipfs-core#282

@alanshaw
Copy link
Contributor

@hacdias did you see the comments here ipfs-inactive/interface-js-ipfs-core#282 (comment) ?

@hacdias
Copy link
Contributor Author

hacdias commented Mar 11, 2019

@alanshaw I believe I missed it!! Just read the comments. I'll work on those points. I'll close this PR as soon as I open a new one since this is a bit out of date.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants