Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

feat(inode): add inode struct #12

Merged
merged 4 commits into from
Dec 11, 2018
Merged

feat(inode): add inode struct #12

merged 4 commits into from
Dec 11, 2018

Conversation

overbool
Copy link
Contributor

@overbool overbool commented Sep 25, 2018 •
edited by schomatis
Loading

Closes: #11

Closes: ipfs/kubo#5182
Closes: ipfs/kubo#5094

@overbool
Copy link
Contributor Author

@schomatis could u help me review it?

@schomatis
Copy link
Contributor

This looks good, as part of removing the inode interface I would also remove the accessor functions (DagService, Parent, Name) since they offer no additional logic (as was suggested to me in the original PR) allowing to access the inode attributes like file.DagService (instead of calling file.DagService()); moving also the function comments to the attributes in the structure.

@overbool
Copy link
Contributor Author

@schomatis Yes, It's really unnecessary to call function. I had fixed it according to your suggestion.

schomatis
schomatis reviewed Sep 25, 2018
View reviewed changes
inode.go Outdated
// and `Directory`. All of its attributes are initialized at
// creation.
type inode struct {
name string
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please put here the comments that were in the removed accessor functions? That way we could leverage them to document this structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@schomatis it's ok, I had added the comments here.

schomatis
schomatis approved these changes Oct 4, 2018
View reviewed changes
Copy link
Contributor

@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.

LGTM, would like to get @magik6k green light before merging since I already messed up this PR twice before.

@schomatis schomatis requested a review from magik6k October 4, 2018 23:18
@Stebalien
go fmt
06fd27e
@Stebalien
avoid unecessary constructors
0ae12b2 
(also avoid exposing a public constructor for a private datastructure)
@Stebalien
avoid unecessary indirection
ec0acc9
Stebalien
Stebalien suggested changes Dec 11, 2018
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Sorry, I never saw this change. I have some nits in overbool#1 but LGTM otherwise.

@ghost ghost assigned Stebalien Dec 11, 2018
@ghost ghost added the status/in-progress In progress label Dec 11, 2018
Stebalien
Stebalien approved these changes Dec 11, 2018
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

(I'm going to forget about this if I don't merge it now so... I'm going to merge it now)

@Stebalien Stebalien merged commit 014cd67 into ipfs:master Dec 11, 2018
@ghost ghost removed the status/in-progress In progress label Dec 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@schomatis schomatis schomatis approved these changes

@Stebalien Stebalien Stebalien approved these changes

@magik6k magik6k Awaiting requested review from magik6k

Assignees

@Stebalien Stebalien

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@overbool @schomatis @Stebalien