feat(analysis): respect.gitignore when walking directories#652
feat(analysis): respect.gitignore when walking directories#652akashchakrabortymsc-cmd wants to merge 14 commits intostjude-rust-labs:mainfrom
Conversation
|
The make_relative_to_with_symlinks test failure looks like a pre-existing Windows symlink permission issue and is not related to this PR. All tests in wdl-analysis pass including the new it_respects_gitignore test. |
There was a problem hiding this comment.
Thanks for working on this — the core change (passing .git_ignore() to WalkBuilder) is on the right track. A few things I think should be addressed before this is ready:
-
Unrelated changes. The
Arena.tomldiff removes hundreds of ShellCheck diagnostics, and the CHANGELOG reformats*→-throughout. These aren't related to the.gitignorefeature and make the diff hard to review. Could you revert those files so the PR only contains the.gitignorechanges? -
Default value. Issue #595 reports that sprocket should respect
.gitignoreby default. Right now the default isfalse(opt-in). I think the default should betrueso that users get the expected behavior without extra configuration. -
CLI integration. The config field exists but there's no way to toggle it from
sprocket checkorsprocket linton the command line. It would be good to either wire it up as a CLI flag. -
Doc comments. The doc comment on
with_respect_gitignorehas an incomplete sentence ("the file walker will skip" — skip what?) and uses'instead of`in theSeelink. Small things, but worth fixing.
The test you added looks good. Looking forward to the next iteration!
|
@claymcleod Thank you sir for the review. Let me know if anything else needs changing! |
claymcleod
left a comment
There was a problem hiding this comment.
I just realized this: the .sprocketignore support is hardcoded—there's no config toggle or CLI flag to disable it. I'd suggest following the same pattern here: remove respect_gitignore from Config/ConfigInner, remove the --no-gitignore CLI flag and Analysis::no_gitignore plumbing, and just hardcode .git_ignore(true) in the walker. That should simplify things quite a bit and it should be ready to merge after—nice work on the feature, and sorry I didn't catch this earlier!
Co-authored-by: Clay McLeod <3411613+claymcleod@users.noreply.github.com>
Co-authored-by: Clay McLeod <3411613+claymcleod@users.noreply.github.com>
Co-authored-by: Clay McLeod <3411613+claymcleod@users.noreply.github.com>
|
@claymcleod kindly review and let me know if their anything to change. |
There was a problem hiding this comment.
Hey — a few things still need to be addressed here:
-
The simplification I suggested hasn't been applied. In my last review, I asked to just hardcode
.git_ignore(true)in the walker (the same pattern we use for.sprocketignore) and removerespect_gitignorefromConfig/ConfigInner, the--no-gitignoreCLI flag, and theAnalysis::no_gitignoreplumbing. The current PR still has all of that configuration machinery. -
The merge with main broke
Arena.toml. The merge commit resolved the conflict by keeping your branch's version ofArena.toml, which had most ShellCheck diagnostics removed. Main has 146 ShellCheck entries; your branch has 90. You'll need to re-merge main and keep main's version ofArena.toml. This file should not show up in the diff when it's complete. -
The CHANGELOG bullet reformatting is still present. The existing convention uses
*, and the diff still changes them to-throughout the file. Please revert those formatting changes and only add your new entry.
Please make sure all of these are addressed before requesting re-review.
f201cd7 to
acdf430
Compare
|
Hey @claymcleod — I've addressed all the feedback from the latest review! |
akashchakrabortymsc-cmd
left a comment
There was a problem hiding this comment.
Resolve the issue
Fixes: #595
This PR adds support for respecting .gitignore files when sprocket check or sprocket lint walks directories .wdl files .
Previously the file walker in wdl-analysis did not respect .gitignore files, causing sprocket to analyze files that user had intentionally excluded from git tracking.
The change adds a respect_gitignore Boolean field to config which when enabled passes .git_ignore(true) to the WalkBuilder in analyzer.rs
A test was added to verify that files listed in .gitignore are correctly skipped during analysis.
For all contributors:
nextbranch in the sprocket.bio repository (when appropriate).For PRs containing lint rule changes:
RULES.md.crates/wdl-lint/tests/lintsthat covers everypossible diagnostic emitted for the rule within the file where the rule
is implemented.