Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mfs list command api return types #5026

Open
achingbrain opened this issue May 18, 2018 · 7 comments
Open

mfs list command api return types #5026

achingbrain opened this issue May 18, 2018 · 7 comments

Comments

@achingbrain
Copy link
Member

achingbrain commented May 18, 2018

The interface-ipfs-core mfs ls command tests seem to be based on the go implementation's behaviour but I'd like to know if the below was intentional.

If you ipfs.files.ls('/directory') you get this sort of structure back:

[
  { name: 'my-file.txt', type: 0, size: 0, hash: '' },
  { name: 'my-directory', type: 0, size: 0, hash: '' }
]

If you ipfs.files.ls('/directory', {l: true}), you get:

[
   {
    name: 'my-file.txt',
    type: 0,
    size: 13,
    hash: 'QmcZojhwragQr5qhTeFAmELik623Z21e3jBTpJXoQ9si1T'
  },
  {
     name: 'my-directory',
     type: 1,
     size: 0,
     hash: 'QmaSPtNHYKPjNjQnYX9pdu5ocpKUQEL3itSz8LuZcoW6J5'
  }
]

In the first example you get type: 0, size: 0, hash: '' which look like they should be omitted from the response.

In the second, the type field has a numeric value instead of file or directory which conflicts with the spec.

Also, has anyone got an opinion about having long be the option field name instead of l? Seems a bit more self-explanatory.

@travisperson
Copy link
Member

This is being addressed at the moment through the following PRs.

ipfs-inactive/js-ipfs-http-client#776
ipfs-inactive/interface-js-ipfs-core#282

However, the actual http api will still return Type as an integer.

@Mr0grog
Copy link
Contributor

Mr0grog commented May 24, 2018

In the second, the type field has a numeric value instead of file or directory which conflicts with the spec.

Hmmm, looking at the API docs, which predate the interface-ipfs-core spec, I think the spec may be wrong (or the intention was never that the schema of the output from the JS library should match the schema of the HTTP API). Or it is painting a picture of an ideal API that doesn’t exist. The ls command has the same problem.

That said, it is super odd that some commands output strings for file types (file.ls, files.stat) and some output ints (ls, files.ls). I had never noticed that before. I’ll bet that inconsistency is how they got written wrong in the spec.

@alanshaw
Copy link
Member

Is my-directory.txt a directory?

@Mr0grog
Copy link
Contributor

Mr0grog commented May 26, 2018

I'd like to know if the below was intentional.

I talked with @whyrusleeping on IRC about the fact that ls and files.ls both use ints, but different ints — he says there wasn’t much thought behind that (he just didn’t consider aligning the values with the non-MFS commands when he made the MFS ones), and agrees that the fact that they are different is confusing. So it’s intentional, but maybe not purposeful.

  • I didn’t think to ask about the strings vs. the ints, but I suspect it’s a similar story.

  • Same for the fact that the responses always have the same structure, regardless of whether values are filled in — these seems like it’s just a matter of the Go implementation using the same value type for both responses (though it seems like this makes things simpler for strongly typed clients).

@whyrusleeping, can fill in anything about either of the above points? ^

has anyone got an opinion about having long be the option field name instead of l? Seems a bit more self-explanatory.

I think that’s a much better name, but the files commands are explicitly trying to match Unix commands, and ls only supports -l and not --long. So I think we’d need to add it in addition rather than instead of.


@whyrusleeping also suggested that, while it might be hard to change them, it’s still better to do so now than later if we think it’s worthwhile. So maybe this is a good opportunity to align all the places in the API where we talk about file node types?

  1. Are they ints or strings?
  2. Can the be the same set of ints/strings (i.e. even if MFS only supports files and directories, can it use the same values as UnixFS for them)?
  3. Can we either:
    • Only send the fields that are filled in. e.g. files/ls without the l would return:

      [{name: 'my-file.txt'}]
    • Use invalid values for fields that aren’t filled in, e.g. -1 for Size and Type (if it’s an int). I suppose the empty string is already clearly invalid for Hash.

The endpoints in question:

  • ls uses ints from UnixFS [0, 1, 2, 3, 4, 5]
  • file.ls (deprecated) uses strings from UnixFS [Raw, Directory, File, Metadata, Symlink, HAMTShard]
  • files.ls uses ints from MFS [0, 1]
  • files.stat uses lower-case strings (that aren’t even constants 😬) that match MFS types [file, directory]

Hope I’m not hijacking and expanding this too much.

@achingbrain
Copy link
Member Author

achingbrain commented May 26, 2018

though it seems like this makes things simpler for strongly typed clients

I'm sure this is possible with Go too, but languages with type systems that I've worked with before have hints you can give to their JSON marshallers/unmarshallers as to how to handle missing/null/empty values if you don't want to use different types for endpoints with more concise output - for example whether to omit them if null or empty while marshalling or to set to a default value when unmarshalling, etc.

So I think we’d need to add it in addition rather than instead of.

Having the CLI tools mimic existing familiar Unix commands is a great design goal as it's instantly familiar for new users but I think our APIs and their arguments should have descriptive names as since code is read a lot more than it is written, being self documenting is more useful than being quick to type.

Are they ints or strings?..

Having magic numbers and occasionally invalid/inconsistent magic numbers usually results in a poor developer experience. Obviously internally you can go to town but at system boundaries like APIs, strings, while slightly more inefficient, are much easier to grok.

Can we either:

I have a strong preference for 3i in your suggestions (e.g. [{name: 'my-file.txt'}] with the non-filled in fields omitted). Sending invalid data, even 'known' invalid data as a side effect of having a type system seems like the tail wagging the dog.

Hope I’m not hijacking and expanding this too much.

Not at all, I think this is a very useful discussion. Thanks for reviewing all the UnixFS related endpoints, I hadn't even noticed the difference between ls ints and files.ls ints (see magic number comment above). It will be great to bring these all into alignment.

@Mr0grog
Copy link
Contributor

Mr0grog commented May 27, 2018

So I think we’d need to [a long option] in addition rather than instead of [the l option].

I think our APIs and their arguments should have descriptive names as since code is read a lot more than it is written, being self documenting is more useful than being quick to type.

I totally agree on this from a philosophical perspective. To be clear, my point was more practical: the whole API infrastructure in Go is built on a system that takes a command definition and makes it available over multiple protocols. It doesn’t provide a facility for marking an option as only being available on one transport (unless I’m missing something). It could always be built! But basically, asking for l to work in CLI but not in HTTP and vice-versa for long is potentially a lot of work involving coordinated releases of up to 3 libraries.

(Additionally, our terribly inconsistent and poorly updated docs [which, yes, I am working on!] have one thing going for them, which is: if the docs don’t make sense, you can always type ipfs xyz --help in your terminal and know those same options are available in the HTTP API. That goes away if we start making distinctions like this.)

Are they ints or strings?

Having magic numbers and occasionally invalid/inconsistent magic numbers usually results in a poor developer experience. Obviously internally you can go to town but at system boundaries like APIs, strings, while slightly more inefficient, are much easier to grok.

  • I think we can all agree inconsistent numbers (or strings!) are not good. No debate there.

  • Just playing devil’s advocate with what feels like the obvious counter-argument: it seems like the most consistent thing would be to use the same values throughout everything. UnixFS is already a public protocol/API, even if poorly documented (so it’s not like the numbers are totally magical/arbitrary), and is also the least malleable part of the system (it would be a rough change to modify the format before v2), so it seems obvious that if we wanted to use the same values everywhere, we’d use UnixFS’s values, and those values are ints. This way, someone’s knowledge of the HTTP API can translate directly to learning to work with raw blocks/objects/nodes (and vice-versa).

@whyrusleeping
Copy link
Member

Hey all, sorry for the confusion. I have been known to write code without thinking hard enough about its consequences, and this is one of those places.

As usual, @Mr0grog's comments are on point. There are a couple ways forward here:

  • Break all existing api users of ipfs files ls -l and switch the magic numbers to match unixfs constants.
  • Add a new field to the ls output that contains a string constant value to describe what the thing is. Return this as well as the integer type. Label the integer type as deprecated for a while, then drop it at some point.
  • same as the above, but use unixfs integer constants instead of a new string constant.

has anyone got an opinion about having long be the option field name instead of l? Seems a bit more self-explanatory.

We can add --long, that should make everyone happy.

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

No branches or pull requests

5 participants