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

Rename ErrSkipDesc to SkipNode #634

Closed
shizhMSFT opened this issue Nov 6, 2023 · 7 comments · Fixed by #643
Closed

Rename ErrSkipDesc to SkipNode #634

shizhMSFT opened this issue Nov 6, 2023 · 7 comments · Fixed by #643
Assignees
Labels
good first issue Good for newcomers
Milestone

Comments

@shizhMSFT
Copy link
Contributor

shizhMSFT commented Nov 6, 2023

#631 exports errSkipDesc to ErrSkipDesc so that copy can be skipped for mounting. However, ErrSkipDesc is not a real error that should be returned by any function.

I have looked into the golang built-in package io/fs and found SkipAll and SkipDir. To have consistent naming as the build-in packages, I propose that we should rename ErrSkipDesc to SkipDesc SkipNode (as discussed in comment section).

var SkipAll = errors.New("skip everything and stop the walk")
var SkipDir = errors.New("skip this directory")

@Wwwsylvia @ktarplee Any comments?

@shizhMSFT shizhMSFT added this to the v2.4.0 milestone Nov 6, 2023
@ktarplee
Copy link
Contributor

ktarplee commented Nov 6, 2023

@shizhMSFT I do like the name SkipDesc better than ErrSkipDesc but maybe the abbreviation needs to be dropped as well. So I actually prefer SkipDescriptor for clarity.

@shizhMSFT
Copy link
Contributor Author

SkipDescriptor sounds good to me.

@ktarplee
Copy link
Contributor

ktarplee commented Nov 7, 2023

Maybe SkipNode is a better name since we are actually skipping the node described by the descriptor and not the descriptor itself.

@sajayantony
Copy link
Contributor

For API choices it always easier when we look at it in the context of code. Can we write up a small code block that would use SkipNode or skipDescriptor?

@ktarplee
Copy link
Contributor

ktarplee commented Nov 8, 2023

For API choices it always easier when we look at it in the context of code. Can we write up a small code block that would use SkipNode or skipDescriptor?

You see a use of ErrSkipDesc in #632

@shizhMSFT
Copy link
Contributor Author

The name SkipNode sound more reasonable to me as we are copying graphs.

@Wwwsylvia
Copy link
Member

SkipNode sounds good to me.

@shizhMSFT shizhMSFT added the good first issue Good for newcomers label Nov 9, 2023
@shizhMSFT shizhMSFT changed the title Rename ErrSkipDesc to SkipDesc Rename ErrSkipDesc to SkipNode Nov 10, 2023
Wwwsylvia pushed a commit that referenced this issue Nov 21, 2023
Resolves #634
Signed-off-by: Xiaoxuan Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants