Recursive#70
Merged
Merged
Conversation
Closed
Reviewer's GuideIntroduces a recursive traversal option for the Snaggle tool by extending its Option API, wiring a new CLI flag, updating the core Snaggle implementation to use WalkDir when requested, and refactoring tests and testdata to validate both flat and recursive modes. Sequence diagram for recursive directory traversal in SnagglesequenceDiagram
actor User
participant CLI
participant Snaggle
participant FileSystem
User->>CLI: Run Snaggle with --recursive flag
CLI->>Snaggle: Call Snaggle(path, root, Recursive())
Snaggle->>FileSystem: WalkDir(path, snagit)
loop For each file/subdirectory
FileSystem->>Snaggle: Provide DirEntry
Snaggle->>Snaggle: Process file (snaggle)
end
Snaggle-->>CLI: Return result
Class diagram for updated options and Option APIclassDiagram
class options {
bool inplace
bool recursive
}
class Option {
<<function>>
}
Option <|.. Inplace
Option <|.. Recursive
class Inplace {
+Inplace() Option
}
class Recursive {
+Recursive() Option
}
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #70 +/- ##
==========================================
- Coverage 75.42% 75.36% -0.07%
==========================================
Files 8 8
Lines 472 483 +11
==========================================
+ Hits 356 364 +8
- Misses 83 85 +2
- Partials 33 34 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `internal/testpaths.go:44` </location>
<code_context>
func WorkspaceTempDir(t testing.TB) string {
t.Helper()
- tmpRoot := TestdataPath(".tmp")
+ tmpRoot := TestdataPath("../../.tmp")
err := os.Mkdir(tmpRoot, os.ModePerm)
if err != nil && !errors.Is(err, os.ErrExist) {
</code_context>
<issue_to_address>
**question (testing):** Changing the temp directory location may affect test isolation and cleanup.
Please verify that using a higher-level temp directory does not cause test interference, file residue, or permission problems, particularly in CI.
</issue_to_address>
### Comment 2
<location> `snaggle.go:181` </location>
<code_context>
+ return nil
+ }
+
+ switch {
+ case options.recursive:
+ return filepath.WalkDir(path, snagit)
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring to use a single WalkDir call with fs.SkipDir to unify directory traversal and error handling.
You can collapse both branches into a single WalkDir call by using fs.SkipDir to prevent descending when not in recursive mode. This removes the manual ReadDir loop and the duplicated error‐handling logic:
```go
func Snaggle(path string, root string, opts ...Option) error {
// … setup options, binDir, libDir …
stat, err := os.Stat(path)
if err != nil {
return err
}
// if it’s just a file, we handle it directly
if !stat.IsDir() {
return snaggle(path, binDir, libDir, options)
}
// directory traversal for both recursive & non-recursive
base := path
return filepath.WalkDir(base, func(p string, d fs.DirEntry, err error) error {
if err != nil {
return err
}
// skip subdirs if not in recursive mode
if d.IsDir() && p != base && !options.recursive {
return fs.SkipDir
}
// ignore all dirs
if d.IsDir() {
return nil
}
// process file
err = snaggle(p, binDir, libDir, options)
var badelf *debug_elf.FormatError
if errors.As(err, &badelf) {
// not an ELF, keep going
return nil
}
return err
})
}
```
What changed:
- Always use a single WalkDir
- Skip descending into subdirectories when !options.recursive
- Only one copy of the “snaggle + FormatError” logic instead of two branches
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #58
Summary by Sourcery
Add a recursive traversal option to the library and CLI, refactor option handling, and extend test coverage with new test data and scenarios for flat versus recursive directory processing.
New Features:
Enhancements:
Build:
Tests:
Chores: