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

fix: files ls tests #282

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

fix: files ls tests #282

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions js/src/files-mfs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,8 +406,8 @@ module.exports = (common) => {
ipfs.files.ls('/test', (err, info) => {
expect(err).to.not.exist()
expect(info).to.eql([
{ name: 'b', type: 0, size: 0, hash: '' },
{ name: 'lv1', type: 0, size: 0, hash: '' }
{ name: 'b', type: 'file', size: 0, hash: '' },
{ name: 'lv1', type: 'file', size: 0, hash: '' }
Copy link
Contributor

Choose a reason for hiding this comment

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

type: 0, size: 0, hash: '' sent by Go regardless - it's unfortunate that type is a valid "type" for the item.

Basically you have to specify the long option to ls in order get these values otherwise you get type: 0, size: 0, hash: '' independent of whether the thing is a file or directory.

You've added 'file' here for the item that is actually a directory and I'd argue that's even more confusing than 0! 🤣

@hacdias - if you're still up for the challenge (I know this PR is old now) I think that we can usefully do is:

  1. Have these properties removed from the core MFS API when you do not specify the long option
  2. Ensure they have the correct type value (string of "directory" or "file") when you do specify long
  3. Keep HTTP API compatibility with Go
    • still reply type: 0, size: 0, hash: '' if no long option is set
    • reply with type as a number in the response if long option IS set
    • have js-ipfs-api do the right thing and normalise the response, removing type: 0, size: 0, hash: '' or converting type to the correct string

BTW, it might be worth closing this PR and opening a new one as things have moved around in this repo significantly!

@achingbrain does that sound ok by you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm starting to think we should make the APIs do the right thing and submit a PR to go-ipfs to make it do the right thing instead of trying to maintain compatibility with an API that is something of a footgun.

In this case long should not even be an option since when you are listing the files internally you have all the information you need to fill those fields in the response. If the CLI wants to throw some of that information away it's essentially UI concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, @alanshaw @achingbrain how should I proceed then? I can remove the properties from the core MFS and ensure they have the correct value for type, keeping the HTTP API compatibility. Or I can always return everything independently of the long option.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alanshaw please advise :)

])
done()
})
Expand All @@ -424,13 +424,13 @@ module.exports = (common) => {
expect(info).to.eql([
{
name: 'b',
type: 0,
type: 'file',
size: 13,
hash: 'QmcZojhwragQr5qhTeFAmELik623Z21e3jBTpJXoQ9si1T'
},
{
name: 'lv1',
type: 1,
type: 'directory',
size: 0,
hash: 'QmaSPtNHYKPjNjQnYX9pdu5ocpKUQEL3itSz8LuZcoW6J5'
}
Expand Down