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

pbdagreader: use FSNode instead of protobuf structure #5189

Merged
merged 2 commits into from
Jul 16, 2018
Merged

pbdagreader: use FSNode instead of protobuf structure #5189

merged 2 commits into from
Jul 16, 2018

Conversation

schomatis
Copy link
Contributor

@schomatis schomatis commented Jul 5, 2018

Addresses part of #5192.


pbdagreader: use FSNode instead of protobuf structure

Focus on the UnixFS layer and avoid explicit references to protocol buffers
format (used to serialize objects of that layer). Use the `unixfs.FSNode`
structure which it abstracts from the `unixfs.pb.Data` format.

Replace `PBDagReader` field `ftpb.Data` with `ft.FSNode`, renaming it to `file`
(which is the type of UnixFS object represented in the reader) and changing its
comment removing the "cached" reference, as this structure is not used here as a
cache (`PBDagReader` doesn't modify the DAG, it's read-only). Also, removed
unused `ProtoNode` field to avoid confusions, as it would normally be present if
the `FSNode` was in fact used as a cache of the contents of the `ProtoNode`.

An example of the advantage of shifting the focus from the format to the UnixFS
layer is dropping the of use `len(pb.Blocksizes)` in favor of the more clear
`NumChildren()` abstraction.

Added `BlockSize()` accessor.

unixfs: remove `Get` prefix from `FSNode` accessors

See https://golang.org/doc/effective_go.html#Getters.

@schomatis schomatis added topic/technical debt Topic technical debt topic/UnixFS Topic UnixFS labels Jul 5, 2018
@schomatis schomatis added this to the Files API Documentation milestone Jul 5, 2018
@schomatis schomatis self-assigned this Jul 5, 2018
@schomatis schomatis requested a review from Kubuxu as a code owner July 5, 2018 02:19
@ghost ghost added the status/in-progress In progress label Jul 5, 2018
Focus on the UnixFS layer and avoid explicit references to protocol buffers
format (used to serialize objects of that layer). Use the `unixfs.FSNode`
structure which it abstracts from the `unixfs.pb.Data` format.

Replace `PBDagReader` field `ftpb.Data` with `ft.FSNode`, renaming it to `file`
(which is the type of UnixFS object represented in the reader) and changing its
comment removing the "cached" reference, as this structure is not used here as a
cache (`PBDagReader` doesn't modify the DAG, it's read-only). Also, removed
unused `ProtoNode` field to avoid confusions, as it would normally be present if
the `FSNode` was in fact used as a cache of the contents of the `ProtoNode`.

An example of the advantage of shifting the focus from the format to the UnixFS
layer is dropping the of use `len(pb.Blocksizes)` in favor of the more clear
`NumChildren()` abstraction.

Added `BlockSize()` accessor.

License: MIT
Signed-off-by: Lucas Molas <schomatis@gmail.com>
See https://golang.org/doc/effective_go.html#Getters.

License: MIT
Signed-off-by: Lucas Molas <schomatis@gmail.com>
@schomatis schomatis added need/review Needs a review and removed status/in-progress In progress labels Jul 6, 2018
@schomatis schomatis changed the title [WIP] pbdagreader: use FSNode instead of protobuf structure pbdagreader: use FSNode instead of protobuf structure Jul 8, 2018
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.

Thats a bit nicer

@schomatis schomatis added RFM and removed need/review Needs a review labels Jul 9, 2018
@whyrusleeping whyrusleeping merged commit 61d08ee into ipfs:master Jul 16, 2018
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