Skip to content
This repository was archived by the owner on Oct 23, 2022. It is now read-only.

add /cat endpoint #184

Merged
merged 45 commits into from
Jun 12, 2020
Merged

add /cat endpoint #184

merged 45 commits into from
Jun 12, 2020

Conversation

koivunej
Copy link
Collaborator

@koivunej koivunej commented Jun 9, 2020

Initial version passes 9/13 tests in the interface-ipfs-core after ipfs/js-ipfs#3078 which are pretty much all of the tests which doesn't use ipfspath.

Follow-up of #176, part of #172. Test changes rs-ipfs/ipfs-rust-conformance#23.

Joonas Koivunen added 2 commits June 9, 2020 12:27
@koivunej
Copy link
Collaborator Author

koivunej commented Jun 9, 2020

Now a new failure: SIGABRT on macos! We have so far had SIGSEGV so we still have a few signals to go until bingo.

@koivunej
Copy link
Collaborator Author

koivunej commented Jun 10, 2020

Big part of the changes I'll push tomorrow next is learning more about IpfsPath. I still haven't moved it over to ipfs from ipfs-http but other than the required error context I can't see many blockers for that. I've re-implemented all dag-pb path walking in the ipfs-unixfs as a precursor to listing all directories. The HAMT sharding walk is BFS until a suitable link is found. I think this could be made better but not on this PR, luckily I can't see how the API should change with the better implementation.

Listing the directories shouldn't be more than refactoring the common parts of ipfs_unixfs::dir::resolve and ipfs_unixfs::dir::ShardLookup to something which will allow listing all (Cid, String). There's still way to go to /get from that though.

LOC wise the biggest change is probably the error context not to introduce yet more strings into interface-ipfs-core.

@koivunej
Copy link
Collaborator Author

Great that we have the conformance tests running on CI as I was only running the related cat tests.

Joonas Koivunen added 5 commits June 11, 2020 13:03
the remaining issue is how to handle the path resolving for the ugly
error mangling case...
also: the walk_path should become ipfs::resolve(IpfsPath).
pub name: Cow<'static, str>,
/// Error from the attempted conversion
pub source: cid::Error,
/// This is to deny creating these outside of the crate
Copy link
Member

Choose a reason for hiding this comment

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

unless we want to expose all the other members outside the crate, making them only pub(crate) would achieve this (just making sure it's not the case)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My idea was to expose the fields and deny creating these values from outside. Hopefully people would use the fmt::Debug printout ... which will be quite the horror to read, should customize it. And/or perhaps I should make the fmt::Display contain everything in case this ever comes up in a bug report.

Copy link
Member

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

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

LGTM, just left a few comments regarding some minor stuff.

koivunej and others added 6 commits June 11, 2020 18:55
Co-authored-by: ljedrz <ljedrz@users.noreply.github.com>
Forgot this from the first batch..

Co-authored-by: ljedrz <ljedrz@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants