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

wants to merge 1 commit into from

Conversation

hacdias
Copy link
Contributor

@hacdias hacdias commented May 23, 2018

{ 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 :)

@victorb victorb removed their request for review April 26, 2019 13:11
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