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

Add boolean value to type assertion, for skipping assertion panic #4800

Closed
wants to merge 1 commit into from
Closed

Add boolean value to type assertion, for skipping assertion panic #4800

wants to merge 1 commit into from

Conversation

ridewindx
Copy link
Contributor

License: MIT
Signed-off-by: Jason Chang ridewindx@163.com

License: MIT
Signed-off-by: Jason Chang ridewindx@163.com
@ridewindx ridewindx requested a review from Kubuxu as a code owner March 10, 2018 12:15
@Stebalien
Copy link
Member

Where did you run across this issue? The parent should always be a pointer to a directory, as far as I know.

@ridewindx
Copy link
Contributor Author

@Stebalien the parent field of Directory may be the *Root:
rval, err := NewDirectory(parent, node.String(), node, root, ds)
https://github.com/ipfs/go-ipfs/blob/5622cc5bb34ddccd2eee7116700bbaf81940d8d4/mfs/system.go#L93
But *Root is obviously not a *Directory. Uh, is that right?

@Stebalien
Copy link
Member

Oh. I see. Um, yeah... that can't currently do anything but panic (it's currently dead code). Thanks for catching this!

I'd rather use a type switch and panic if the parent is neither a directory nor root:

switch parent := cur.parent.(type) {
case *Directory:
	cur = parent
	// ...
case *Root:
	return path
default:
	panic("directory parent neither a directory nor a root")
}

Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

Could you also add simple unit test for it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants