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

mfs: add Inode interface #5182

Closed
wants to merge 1 commit into from
Closed

mfs: add Inode interface #5182

wants to merge 1 commit into from

Conversation

schomatis
Copy link
Contributor

@schomatis schomatis commented Jul 3, 2018

As suggested in #5094 (comment).


mfs: add `Inode` interface

Abstract common characteristics of the MFS `File` and `Directory` structures
inside a separate interface called `Ìnode`.

Closes #5094.

@schomatis schomatis requested a review from Kubuxu as a code owner July 3, 2018 12:57
@ghost ghost assigned schomatis Jul 3, 2018
@ghost ghost added the status/in-progress In progress label Jul 3, 2018
@schomatis schomatis added this to the Files API Documentation milestone Jul 3, 2018
@schomatis schomatis added need/review Needs a review topic/MFS Topic MFS and removed status/in-progress In progress labels Jul 3, 2018
@ghost ghost added the status/in-progress In progress label Jul 3, 2018
@schomatis schomatis removed the status/in-progress In progress label Jul 3, 2018
mfs/inode.go Outdated
}

// GetName returns the `Inode`'s name.
func (i *Inode) GetName() string {
Copy link
Member

Choose a reason for hiding this comment

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

Why not access these directly? Would be cleaner imo (and look less than java)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not access these directly?

To make it explicit that it is a read-only attribute and should not be modified.

(and look less than java)

Would you prefer just using Name()?

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

@schomatis schomatis added status/in-progress In progress and removed need/review Needs a review labels Jul 6, 2018
@schomatis schomatis changed the title mfs: add Inode structure [WIP] mfs: add Inode structure Jul 6, 2018
@schomatis
Copy link
Contributor Author

schomatis commented Jul 6, 2018

To make it explicit that it is a read-only attribute and should not be modified.

This is actually not happening right now in this PR, parent is being directly accessed and I didn't even noticed it. I need to rethink this, adding Inode as a named field to protect its members would look ugly: d.inode.parent().closeChild().

https://stackoverflow.com/a/32535022

@schomatis
Copy link
Contributor Author

As mentioned in the Stack Overflow answer, the customary Go solution seems to be to define an interface (Inode) and a separate structure to implement it.

@schomatis schomatis changed the title [WIP] mfs: add Inode structure mfs: add Inode interface Jul 12, 2018
Abstract common characteristics of the MFS `File` and `Directory` structures
inside a separate interface called `Ìnode`.

License: MIT
Signed-off-by: Lucas Molas <[email protected]>
@schomatis schomatis added need/review Needs a review and removed status/in-progress In progress labels Jul 12, 2018
}

// inode implements the `Inode` interface.
type inode struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the appropriate name for the structure that implements the Inode interface?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, thats what I would do.

@schomatis schomatis requested a review from Stebalien July 19, 2018 04:44
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.

Given that this is just shared data (no logic), I'd just use a struct (and embed it in each file type). That would deduplicate the fields without adding additional complexity/overhead.

@whyrusleeping
Copy link
Member

Pretty clean refactor, stevens right though, it probably makes sense to just embed a struct here. (unless you want to be able to mock out the inode stuff for some reason in testing)

@schomatis
Copy link
Contributor Author

(unless you want to be able to mock out the inode stuff for some reason in testing)

Not really, my main motivation for the interface was to be able to enforce read-only attributes (more as a code clarification feature than to protect inadvertent modifications to the object), but that may not be the most Go idiomatic approach, I'll embed the structure then.

@schomatis schomatis added status/in-progress In progress and removed need/review Needs a review labels Jul 23, 2018
@schomatis schomatis added the help wanted Seeking public contribution on this issue label Sep 24, 2018
@schomatis
Copy link
Contributor Author

@overbool Want to take over this one?

This should be ported to the go-mfs repo now and the interface (as suggested above) should be turned to a structure to simplify the code.

@overbool
Copy link
Contributor

@schomatis Yes. I'm also interested in this.

@schomatis
Copy link
Contributor Author

Great! Ping me if you have any trouble porting the diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Seeking public contribution on this issue topic/MFS Topic MFS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants