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

Refactoring on filepath.Walk #3885

Merged
merged 3 commits into from
Apr 7, 2020
Merged

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Mar 27, 2020

This is a first step in the refactoring of filepath.Walk and godirwalk.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

@codecov
Copy link

codecov bot commented Mar 27, 2020

Codecov Report

Merging #3885 into master will increase coverage by 0.03%.
The diff coverage is 75.59%.

Impacted Files Coverage Δ
pkg/skaffold/docker/syncmap.go 68.25% <61.90%> (+1.08%) ⬆️
pkg/skaffold/deploy/helm.go 78.08% <66.66%> (+0.30%) ⬆️
pkg/skaffold/build/list/list.go 70.58% <68.00%> (-0.85%) ⬇️
pkg/skaffold/docker/dependencies.go 71.83% <70.58%> (-1.09%) ⬇️
pkg/skaffold/docker/dockerignore.go 75.00% <75.00%> (ø)
pkg/skaffold/walk/walk.go 81.35% <81.35%> (ø)
pkg/skaffold/build/jib/jib.go 72.99% <81.81%> (-1.28%) ⬇️
cmd/skaffold/app/cmd/find_configs.go 45.71% <100.00%> (ø)
pkg/skaffold/util/util.go 85.50% <100.00%> (-0.51%) ⬇️
... and 2 more

@dgageot dgageot changed the title Rectoring on filepath.Walk Refactoring on filepath.Walk Mar 27, 2020
Copy link
Member

@briandealwis briandealwis left a 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?

pkg/skaffold/walk/walk.go Outdated Show resolved Hide resolved
pkg/skaffold/walk/walk.go Outdated Show resolved Hide resolved
pkg/skaffold/walk/walk.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/helm.go Outdated Show resolved Hide resolved
pkg/skaffold/walk/walk.go Outdated Show resolved Hide resolved
pkg/skaffold/walk/walk.go Show resolved Hide resolved
pkg/skaffold/docker/syncmap.go Outdated Show resolved Hide resolved
@dgageot dgageot force-pushed the walk branch 7 times, most recently from 151bf4e to 55ba60e Compare April 1, 2020 14:53
@dgageot
Copy link
Contributor Author

dgageot commented Apr 2, 2020

@briandealwis could you take a second (third?) look

Copy link
Member

@briandealwis briandealwis left a 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?

hack/versions/cmd/new/version.go Show resolved Hide resolved
pkg/skaffold/build/jib/jib.go Outdated Show resolved Hide resolved
pkg/skaffold/docker/dockerignore.go Outdated Show resolved Hide resolved
Copy link
Member

@briandealwis briandealwis left a 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.

pkg/skaffold/docker/dependencies.go Show resolved Hide resolved
pkg/skaffold/walk/walk.go Outdated Show resolved Hide resolved
return godirwalk.Walk(w.dir, &godirwalk.Options{
Unsorted: w.unsorted,
Callback: func(path string, info *godirwalk.Dirent) error {
match, err := w.predicate(path, info)
Copy link
Member

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).

Copy link
Contributor Author

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>
dgageot added 2 commits April 7, 2020 10:05
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Copy link
Member

@briandealwis briandealwis left a 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.

@dgageot dgageot merged commit 61d1bba into GoogleContainerTools:master Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants