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

Upgrade API to comply with the IPFS Interface specification #146

Closed
wants to merge 2 commits into from

Conversation

ntninja
Copy link
Contributor

@ntninja ntninja commented Nov 24, 2018

See https://github.com/ipfs/interface-ipfs-core/tree/master/SPEC for the (JS) spec we're trying to mimic here. Feedback from the authors of that spec (particularly @daviddias @alanshaw) would be highly appreciated. All of these changes are fully backwards-compatible (note that I didn't even touch any tests!).

@ghost ghost assigned ntninja Nov 24, 2018
@ghost ghost added the in progress label Nov 24, 2018
@ntninja ntninja force-pushed the ipfs-interface-spec branch 2 times, most recently from 4368de1 to fa4a433 Compare November 24, 2018 17:18
@ntninja
Copy link
Contributor Author

ntninja commented Dec 5, 2018

@daviddias Please give some feedback on this.

@daviddias daviddias requested review from alanshaw and removed request for daviddias December 5, 2018 17:30
@daviddias
Copy link
Member

@Alexander255 reviewing 2700 LOC changed in a codebase that I never looked much is hard. Could you list out what this PR does? Which funcs it implements? Which ones it doesn't implements? Which ones have tests? What's next? And so ?

@daviddias daviddias self-requested a review December 5, 2018 17:32
@ntninja
Copy link
Contributor Author

ntninja commented Dec 13, 2018

@daviddias: Fair enough I guess 🙂

The main point of this PR is changing the way the existing IPFS API is presented to users of the library. For instance, previously ipfs files cp would have been client.files_cp(…) in Python; with this PR it would become client.files.cp(…) (like in JavaScript). I made these changes in accordance with the ipfs-interface-core spec. In particular:

  • I split the giant client.py file into several roughly along the lines of that spec (but this is not user-visible anyways)
  • I changed method names to be of the client.a.b(…) style where appropriate

(Not that this will also be released as part of the new ipfshttpclient library with the current code being for the old ipfsapi library and including the backward-compatibility shims.)

Some questions, mainly based on the discussions in https://github.com/ipfs/interface-ipfs-core/issues/284:

  • Is the split of the files module into client.{add,file_ls,get,cat,ls} on the one hand vs client.files.{cp,ls,mkdir,mv,read,rm,stat,write} on the hand still current or should things be done differently now that we have a chance to break API? (Remember I'll push an update for the old library adding shims and wrapping the new version.)
  • What's the point of /api/v0/file/ls vs /api/v0/ls anyways? Can one of these be dropped? Found Deprecate ipfs file ls ipfs/kubo#5806: Yes, ipfs file ls is scheduled for removal soon.
  • What's this chat about ipfsx and how does it relate to the exposed interfaces?
  • Is /api/v0/bitswap/unwant dead? It's giving me errors on newer versions of IPFS. Found remove bitswap unwant ipfs/kubo#5308: The short answer is “yes”.
  • Any other feedback you may have.

Hope this is more useful this time 😊

@ntninja ntninja mentioned this pull request Dec 15, 2018
@ntninja ntninja changed the title Upgrade API to comply to the IPFS Interface specification Upgrade API to comply with the IPFS Interface specification Dec 17, 2018
@ntninja
Copy link
Contributor Author

ntninja commented Jan 8, 2019

@daviddias: Ping

@ntninja
Copy link
Contributor Author

ntninja commented Jan 23, 2019

@daviddias: Ping again. I see that js-ipfs recently switched to the new namespace for the {add,get,cat,ls}. This is not reflected in https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/FILES.md however, at the moment.

@daviddias
Copy link
Member

Those changes are part of a bigger revamp of the whole API around files at IPFS. More discussions at:

Lot's of context there ^^

Is the split of the files module into client.{add,file_ls,get,cat,ls} on the one hand vs client.files.{cp,ls,mkdir,mv,read,rm,stat,write} on the hand still current or should things be done differently now that we have a chance to break API? (Remember I'll push an update for the old library adding shims and wrapping the new version.)

The split is due that in go-ipfs, any command that is below ipfs.files is actually acting on top of the MFS subsystem, while any command related to files on ipfs. is just default baseline commands. There is a proposal to merge both.

What's this chat about ipfsx and how does it relate to the exposed interfaces?

It is our experimental platform o make the APIs more ergonomic for JavaScript developers.

@daviddias: Ping again. I see that js-ipfs recently switched to the new namespace for the {add,get,cat,ls}. This is not reflected in https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/FILES.md however, at the moment.

This will be reflected soon on interface-ipfs-core too. I recommend following that for the py-ipfs-api. //cc @alanshaw

@ntninja
Copy link
Contributor Author

ntninja commented Jan 27, 2019

@daviddias: Thanks for your response!

It looks like I misread the release announcement at ipfs/js-ipfs#1721: JS-IPFS moved the non-MFS file APIs from the files.*-namespace to top-level, not the other way round! Meaning that the code now behaves like it was outlined in https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/FILES.md all along, no? (I don't care about the JS-specific details regarding pull-streams et al here since that is JS-stuff not Python-stuff.)

Thank you, I'll do it this way then and wait for further developments down the line, when/if the file.*-namespace and top-level are merged.

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

Successfully merging this pull request may close these issues.

2 participants