Refactoring on filepath.Walk#3885
Conversation
dgageot
requested review from
TadCordle,
balopat,
briandealwis,
chanseokoh,
loosebazooka,
nkubala and
tejal29
as code owners
Codecov Report
|
briandealwis
left a comment
briandealwis
left a comment
There was a problem hiding this comment.
A few nits. The Excluder seems like it should be worked into the Walk interface too?
dgageot
force-pushed
the
walk
branch
7 times, most recently
from
151bf4e to
55ba60e
Compare
|
@briandealwis could you take a second (third?) look |
briandealwis
left a comment
briandealwis
left a comment
There was a problem hiding this comment.
Are the paths provided from gowalkdir always absolute?
briandealwis
left a comment
briandealwis
left a comment
There was a problem hiding this comment.
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.
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.
Yeah, I've seen that too. Maybe in a next PR though?
Signed-off-by: David Gageot <david@gageot.net>
briandealwis
left a comment
briandealwis
left a comment
There was a problem hiding this comment.
LGTM! I think this filter-style is much easier to reason about.
3 participants
This is a first step in the refactoring of
filepath.Walkandgodirwalk.Walkusage.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