-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Refactoring on filepath.Walk #3885
Conversation
Codecov Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits. The Excluder seems like it should be worked into the Walk interface too?
151bf4e
to
55ba60e
Compare
@briandealwis could you take a second (third?) look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the paths provided from gowalkdir always absolute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits in variable naming with regards to being absolute.
return godirwalk.Walk(w.dir, &godirwalk.Options{ | ||
Unsorted: w.unsorted, | ||
Callback: func(path string, info *godirwalk.Dirent) error { | ||
match, err := w.predicate(path, info) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems most of the uses of Walk()
relativize the path against the dir. I wonder if it would make sense to relativize the path here and pass in the relative path into the predicate and action. Or maybe it can be controlled by an option on the builder? walk.From(xxx).RelativePaths().When(yyy).Do(zzz)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've seen that too. Maybe in a next PR though?
Signed-off-by: David Gageot <david@gageot.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I think this filter-style is much easier to reason about.
This is a first step in the refactoring of
filepath.Walk
andgodirwalk.Walk
usage.The idea is to simplify the code that walks directories by extracting as much common code as possible.
More simplifications are coming but this first step is already bringing many simplifications.
Signed-off-by: David Gageot david@gageot.net