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

unixfs: add a directory interface #5160

Merged
merged 2 commits into from
Jul 16, 2018
Merged

unixfs: add a directory interface #5160

merged 2 commits into from
Jul 16, 2018

Conversation

schomatis
Copy link
Contributor

@schomatis schomatis commented Jun 27, 2018

Addresses #5157.
Closes #5084.


unixfs: add a directory interface

Add a UnixFS `Directory` that hides implementation details and helps to
distinguish *what* is a UnixFS directory.

Replace the `unixfs.io.Directory` structure that contained the HAMT and basic
directory implementations (through inner pointers) with an interface containing
the same methods. Implement those methods in two clearly distinct structures for
each implementation (`BasicDirectory` and `HAMTDirectory`) avoiding pointer
logic and clearly differentiating which implementation does what.

The potential basic to HAMT transition was being hidden behind the `AddChild`
call at the UnixFS layer (changing one implementation pointer  with the other
one), it is now being explicitly done at the MFS layer.

Rename the `dirbuilder.go` file to `directory.go` and change the `Directory` MFS
attribute `dirbuilder` to `unixfsDir` to be consistent.

@schomatis schomatis added status/in-progress In progress topic/UnixFS Topic UnixFS labels Jun 27, 2018
@schomatis schomatis added this to the Files API Documentation milestone Jun 27, 2018
@schomatis schomatis self-assigned this Jun 27, 2018
@schomatis schomatis requested a review from Kubuxu as a code owner June 27, 2018 13:33
mfs/dir.go Outdated
@@ -363,6 +363,18 @@ func (d *Directory) AddChild(name string, nd ipld.Node) error {
return err
}

//TODO: Resolve where to manage a possible transition to sharding.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is ugly, and it's not equivalent to transitioning inside unix.io.Directory.AddChild.

mfs/dir.go Outdated
// TODO: Rename. Actually the real confusion is that
// this structure has a `AddChild` function that calls
// another `AddChild` function of its inner dir.
func (d *Directory) AddChildUnixFSDir(name string, nd ipld.Node) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@whyrusleeping What do you think about moving the HAMT transition to the MFS layer?

Copy link
Member

Choose a reason for hiding this comment

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

I guess the answer to that question depends on when we will need to do this, but I think its probably fine to have it in mfs

Copy link
Contributor Author

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

// It just allows to perform explicit edits on a single directory, working with
// directory trees is out of its scope, they are managed by the MFS layer
// (which is the main consumer of this interface).
type Directory interface {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please review these comments and add information here.

return nil, ErrNotADir
}

// SetPrefix implements the `Directory` interface.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm basically repeating "implements the Directory interface" to avoid copying the method comments of the interface (in case they are changed, DRY). I'm only commenting here particularities of each implementation. However, this doesn't seem so nice either, what's the Go idiomatic way of doing it?

Copy link
Member

Choose a reason for hiding this comment

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

I think in go, for obvious interface implementation methods, they often just get left blank. Saying 'implements X' is fine with me too

@schomatis schomatis added need/review Needs a review topic/technical debt Topic technical debt and removed status/in-progress In progress labels Jul 8, 2018
@schomatis schomatis changed the title [WIP] unixfs: add a directory interface unixfs: add a directory interface Jul 8, 2018
// ShardSplitThreshold specifies how large of an unsharded directory
// the Directory code will generate. Adding entries over this value will
// result in the node being restructured into a sharded object.
var ShardSplitThreshold = 1000
Copy link
Member

Choose a reason for hiding this comment

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

I think this is actually unused now. We should probably remove it

// (which is the main consumer of this interface).
type Directory interface {

// SetPrefix sets the prefix of the root node.
Copy link
Member

Choose a reason for hiding this comment

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

i'd say "sets the cid prefix"

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

This LGTM. Thanks a bunch :)

Add a UnixFS `Directory` that hides implementation details and helps to
distinguish *what* is a UnixFS directory.

Replace the `unixfs.io.Directory` structure that contained the HAMT and basic
directory implementations (through inner pointers) with an interface containing
the same methods. Implement those methods in two clearly distinct structures for
each implementation (`BasicDirectory` and `HAMTDirectory`) avoiding pointer
logic and clearly differentiating which implementation does what.

The potential basic to HAMT transition was being hidden behind the `AddChild`
call at the UnixFS layer (changing one implementation pointer  with the other
one), it is now being explicitly done at the MFS layer.

Rename the `dirbuilder.go` file to `directory.go` and change the `Directory` MFS
attribute `dirbuilder` to `unixfsDir` to be consistent.

License: MIT
Signed-off-by: Lucas Molas <[email protected]>
@ghost ghost added the status/in-progress In progress label Jul 9, 2018
@schomatis schomatis added RFM and removed status/in-progress In progress need/review Needs a review labels Jul 9, 2018
@whyrusleeping whyrusleeping merged commit b126601 into ipfs:master Jul 16, 2018
@schomatis schomatis deleted the feat/unixfs/dir-interface branch July 23, 2018 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFM topic/technical debt Topic technical debt topic/UnixFS Topic UnixFS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants