Repository navigation
Add native -exclude and -respect_bazelignore to buildifier - #1530
ArchangelX360 wants to merge 2 commits into
Conversation
This gives cross platform consistency instead of relying on shell / bat scripts that might differ depending on the tools you call.
7b5dc8e to
bc8275f
Compare
Fixes several issues of the commit introducing `-exclude`: * Patterns use the syntax of Bazel's glob(): '*' and '?' match within a single path segment, and '**' as a whole segment matches any number of segments. * Patterns are relative to the workspace root, not the working directory, so that they mean the same thing wherever buildifier runs and in .buildifier.json. * The paths of .bazelignore and the patterns of ignore_directories() in REPO.bazel are excluded by default; -respect_bazelignore=false turns that off. * Excluded paths are checked before walk errors, so unreadable excluded directories do not fail the run.
bc8275f to
01209f8
Compare
| // RespectBazelignore also skips the paths ignored by the workspace's | ||
| // .bazelignore and the ignore_directories() of its REPO.bazel during | ||
| // recursive file discovery | ||
| RespectBazelignore bool `json:"respectBazelignore"` |
There was a problem hiding this comment.
Can we use a different name here? In a future where ignore_directories is much more widely adopted, this name wouldn't fit well. How about -respect_ignores?
| {"vendor/*", "vendor/x", true}, | ||
| {"vendor/*", "vendor/x/y", false}, |
There was a problem hiding this comment.
Including an excluded directory when starting in it seems a bit unintuitive, could you make exclusions independent of where the search starts?
| } | ||
| } | ||
|
|
||
| func TestExcluderRespectsBazelignoreAndIgnoreDirectories(t *testing.T) { |
There was a problem hiding this comment.
These patterns also exclude files in this PR, but don't in Bazel (e.g. vendor/* does not exclude vendor/BUILD.bazel).
| return nil, err | ||
| } | ||
| for _, dir := range ignored { | ||
| if err := e.add(escapeGlob(dir)); err != nil { |
There was a problem hiding this comment.
Bazel would Clean the paths, I think, so we should as well at this point.
| // multi_diff: the command specified by the -diff_command flag can diff multiple files in the style of tkdiff (default false) ("false") | ||
| // path: assume BUILD file has this path relative to the workspace directory ("") | ||
| // r: find starlark files recursively ("false") | ||
| // respect_bazelignore: also exclude the paths ignored by .bazelignore and the ignore_directories() of REPO.bazel when finding starlark files recursively ("true") |
There was a problem hiding this comment.
A number of projects format everything but exclude their e2e or examples directories via .bazelignore, which makes me think we shouldn't make this the default.
| func ExpandDirectories(args *[]string) ([]string, error) { | ||
| // by traversing each directory recursively and searching for relevant Starlark files. Paths | ||
| // that the optional excluder excludes are skipped, without reading excluded directories. | ||
| func ExpandDirectories(args *[]string, excluder *Excluder) ([]string, error) { |
There was a problem hiding this comment.
This (unfortunately) is public API, so please preserve the signature and add a new in an internal subpackage.
|
|
||
| // relativeTo returns the slash-separated path of abs relative to root, if abs | ||
| // is strictly inside root. | ||
| func relativeTo(root, abs string) (string, bool) { |
There was a problem hiding this comment.
This type of operation is usually a code smell and very hard to get right (path normalization, case sensitivity, ...). Can we refactor this so that Excluder operates on slash-separated, relative paths only with a Excluded(rel string, isDir bool) signature? It would be okay for buildifier -r ../other-repo to pick up other-repo's .bazelignore in that case, as I think you would need to determine the root per argument to implement this refactoring.
No description provided.