ipfs / kubo

An IPFS implementation in Go
https://docs.ipfs.tech/how-to/command-line-quick-start/
Other
16.16k stars 3.01k forks source link

mfs list command api return types #5026

Open achingbrain opened 6 years ago

achingbrain commented 6 years ago

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 commented 6 years ago

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

https://github.com/ipfs/js-ipfs-api/pull/776 https://github.com/ipfs/interface-ipfs-core/pull/282

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

Mr0grog commented 6 years ago

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 commented 6 years ago

Is my-directory.txt a directory?

Mr0grog commented 6 years ago

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.

@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:

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

achingbrain commented 6 years ago

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 commented 6 years ago

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.

whyrusleeping commented 6 years ago

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:

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.