Skip to content

Recursive#70

Merged
MusicalNinjaDad merged 11 commits into
mainfrom
recursive
Oct 27, 2025
Merged

Recursive#70
MusicalNinjaDad merged 11 commits into
mainfrom
recursive

Conversation

@MusicalNinjaDad

@MusicalNinjaDad MusicalNinjaDad commented Oct 27, 2025

Copy link
Copy Markdown
Owner

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:

  • Support recursive directory traversal in Snaggle with a --recursive (or -r) flag

Enhancements:

  • Refactor Snaggle function signature to use Option type and streamline directory handling logic

Build:

  • Add buildscript entries for the dynamic hello binary in testdata

Tests:

  • Extend unit and CLI tests to verify both flat and recursive modes and include a new dynamic subdirectory binary

Chores:

  • Adjust WorkspaceTempDir path helper and test data paths

@MusicalNinjaDad MusicalNinjaDad linked an issue Oct 27, 2025 that may be closed by this pull request
@sourcery-ai

sourcery-ai Bot commented Oct 27, 2025

Copy link
Copy Markdown

Reviewer's Guide

Introduces 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 Snaggle

sequenceDiagram
    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
Loading

Class diagram for updated options and Option API

classDiagram
    class options {
        bool inplace
        bool recursive
    }
    class Option {
        <<function>>
    }
    Option <|.. Inplace
    Option <|.. Recursive
    class Inplace {
        +Inplace() Option
    }
    class Recursive {
        +Recursive() Option
    }
Loading

File-Level Changes

Change Details Files
Add recursive snaggling option
  • Introduce Option type alias and add recursive field to options struct
  • Implement Recursive() option function
  • Modify Snaggle() to dispatch between os.ReadDir and filepath.WalkDir based on recursive flag
  • Add -r/--recursive CLI flag and propagate option to Snaggle
  • Update function signature to accept Option variadic
snaggle.go
snaggle/main.go
Refactor tests to validate recursive mode
  • Wrap TestDirectory in flat and recursive subtests in both unit and CLI tests
  • Inject Hello_dynamic into expected binaries map when recursive
  • Update expected error handling and output assertions for recursive case
snaggle_test.go
cli_test.go
Add dynamic Hello test binary
  • Define hello_dynamic entry in commonElfs map
  • Add P_hello_dynamic constant to testpaths
  • Extend build.sh to compile hello_dynamic with CGO
internal/testing/testdata.go
internal/testpaths.go
internal/testdata/hello/build.sh
Fix WorkspaceTempDir path resolution
  • Adjust tmpRoot in WorkspaceTempDir to reside under '../../.tmp' for correct filesystem placement
internal/testpaths.go

Assessment against linked issues

Issue Objective Addressed Explanation
#58 Implement a --recursive command-line flag that enables recursive processing of subdirectories when snaggling a directory.
#58 Update the codebase so that the --recursive flag is correctly parsed and passed to the snaggling logic, affecting directory traversal behavior.
#58 Add or update tests to verify that recursive snaggling works as intended when the --recursive flag is used.

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@codecov

codecov Bot commented Oct 27, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 64.28571% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.36%. Comparing base (e456836) to head (00699cc).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
snaggle/main.go 14.28% 6 Missing ⚠️
snaggle.go 80.00% 2 Missing and 2 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread internal/testpaths.go
@MusicalNinjaDad MusicalNinjaDad merged commit e35048d into main Oct 27, 2025
10 of 12 checks passed
@MusicalNinjaDad MusicalNinjaDad deleted the recursive branch October 27, 2025 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add --recursive

1 participant