Fix some weird code in core/coreunix/add.go #5354
Conversation
ghost
added
the
hsanjuan
force-pushed
the
fix/coreunix/add/improvs
branch
from
3e06c90
to
4c44ef8
Compare
hsanjuan
added
topic/core
| @@ -201,7 +201,8 @@ func (adder *Adder) Finalize() (ipld.Node, error) { | |||
| return nil, err | |||
| } | |||
| var root mfs.FSNode | |||
| root = mr.GetDirectory() | |||
| rootdir := mr.GetDirectory() | |||
| root = rootdir | |||
There was a problem hiding this comment.
Let's leave only one of these definitions, and we can drop the mfs.FSNode definition (it will only be an mfs.Directory), something like:
root := mr.GetDirectory()or rootDir if you want to emphasize that we're dealing with a directory here.
There was a problem hiding this comment.
@schomatis the only way to drop rootdir is to perform type assertions on an the FSNode root. I think unchecked type assertions are places for potential future bugs. Otherwise I have to check the assertions, which is way worse than having a variable with the right type from the beginning.
There was a problem hiding this comment.
root cannot stay as *mfs.Directory type because it gets re-assigned an FSNode which may not be a directory.
There was a problem hiding this comment.
You need to check that the child of the root is a directory, otherwise more potentially harder-to-find bugs could follow if you allow the root to be an mfs.File.
Otherwise I have to check the assertions, which is way worse than having a variable with the right type from the beginning.
I'm failing to see the problem, you could have both, making root here explicitly as a directory and check that the child of this directory is also a directory in the posterior assignment.
There was a problem hiding this comment.
The child of the root does not need to be a directory. It's ok if it's an FSNode. The code is fine. I think this discussion is just about personal preferences.
|
@schomatis actually the weirdness was there before your changes |
hsanjuan
force-pushed
the
fix/coreunix/add/improvs
branch
from
4c44ef8
to
3443fe6
Compare
|
|
||
| dir := mr.GetDirectory() | ||
|
|
||
| root, err = dir.Child(name) |
There was a problem hiding this comment.
We should assert for a directory type here then.
There was a problem hiding this comment.
No, it does not need to be a directory
| @@ -201,7 +201,8 @@ func (adder *Adder) Finalize() (ipld.Node, error) { | |||
| return nil, err | |||
| } | |||
| var root mfs.FSNode | |||
| root = mr.GetDirectory() | |||
| rootdir := mr.GetDirectory() | |||
| root = rootdir | |||
There was a problem hiding this comment.
You need to check that the child of the root is a directory, otherwise more potentially harder-to-find bugs could follow if you allow the root to be an mfs.File.
Otherwise I have to check the assertions, which is way worse than having a variable with the right type from the beginning.
I'm failing to see the problem, you could have both, making root here explicitly as a directory and check that the child of this directory is also a directory in the posterior assignment.
Code was just weird. I think it looks better now. License: MIT Signed-off-by: Hector Sanjuan <hector@protocol.ai>
hsanjuan
force-pushed
the
fix/coreunix/add/improvs
branch
from
3e3a694
to
de72087
Compare
|
@Stebalien this should be RFM |
Code was just weird. I think it looks better now.
License: MIT
Signed-off-by: Hector Sanjuan hector@protocol.ai