-
Notifications
You must be signed in to change notification settings - Fork 165
Conversation
94044c2
to
6823194
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@koivunej This should be mostly ready—I've left some comments for things that I'm unsure of. There are still some 80 (down from 150+) warnings, though I wanted to keep these PRs fairly small given they affect many parts of the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is looking really great! Also thanks for the good questions. I left at least one TBD but I think we should start merging this today-ish.
src/dag.rs
Outdated
/// Creates a new `IpldDag` for DAG operations. | ||
pub fn new(ipfs: Ipfs<Types>) -> Self { | ||
IpldDag { ipfs } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit like an oversight for anyone having to write or read an "MSDN style" piece of documentation.
Looking that
Lines 387 to 389 in 3fc24b0
pub fn dag(&self) -> IpldDag<Types> { | |
IpldDag::new(self.clone()) | |
} |
pub fn
. While the current API is of course far from perfect, perhaps I'd like to suggest this:
/// Creates a new `IpldDag` for DAG operations. | |
pub fn new(ipfs: Ipfs<Types>) -> Self { | |
IpldDag { ipfs } | |
} | |
/// Creates a new `IpldDag` for DAG operations. | |
// FIXME: duplicates Ipfs::dag(), having both is redundant | |
pub fn new(ipfs: Ipfs<Types>) -> Self { | |
IpldDag { ipfs } | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good!
bors r+
Build succeeded: |
Follow up to #409 and part of #197.