-
Notifications
You must be signed in to change notification settings - Fork 85
feat: add --staged diff scope for pre-commit hook scanning (#18) #53
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| package cmd | ||
|
|
||
| import ( | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestDiffOptsValidateStaged(t *testing.T) { | ||
| cases := []struct { | ||
| name string | ||
| opts diffOpts | ||
| wantErr string | ||
| }{ | ||
| {name: "staged only", opts: diffOpts{staged: true, scope: "changed_functions"}}, | ||
| {name: "staged + base", opts: diffOpts{staged: true, base: "origin/main", scope: "changed_functions"}, wantErr: "mutually exclusive"}, | ||
| {name: "staged + pr", opts: diffOpts{staged: true, pr: 9, scope: "changed_functions"}, wantErr: "mutually exclusive"}, | ||
| {name: "base + pr", opts: diffOpts{base: "x", pr: 9, scope: "changed_functions"}, wantErr: "mutually exclusive"}, | ||
| {name: "staged empty scope", opts: diffOpts{staged: true}, wantErr: "must not be empty"}, | ||
| {name: "staged invalid scope", opts: diffOpts{staged: true, scope: "everything"}, wantErr: "invalid --diff-scope"}, | ||
| {name: "nothing set is fine", opts: diffOpts{}}, | ||
| } | ||
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| err := tc.opts.validate() | ||
| if tc.wantErr == "" { | ||
| if err != nil { | ||
| t.Errorf("expected no error, got %v", err) | ||
| } | ||
| return | ||
| } | ||
| if err == nil { | ||
| t.Fatalf("expected error containing %q, got nil", tc.wantErr) | ||
| } | ||
| if !strings.Contains(err.Error(), tc.wantErr) { | ||
| t.Errorf("expected error containing %q, got %q", tc.wantErr, err.Error()) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestDiffOptsIsSetStaged(t *testing.T) { | ||
| if !(diffOpts{staged: true}).isSet() { | ||
| t.Error("staged=true should be isSet()") | ||
| } | ||
| if (diffOpts{}).isSet() { | ||
| t.Error("zero diffOpts should not be isSet()") | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,7 @@ var ( | |
| scanIncremental bool | ||
| scanDiffBase string | ||
| scanPR int | ||
| scanStaged bool | ||
| scanDiffScope string | ||
| ) | ||
|
|
||
|
|
@@ -78,6 +79,7 @@ func registerScanFlags(cmd *cobra.Command) { | |
| cmd.Flags().BoolVar(&scanIncremental, "incremental", false, "Incremental against the last successful scan on this project") | ||
| cmd.Flags().StringVar(&scanDiffBase, "diff-base", "", "Incremental mode: filter pipeline to units overlapping diff vs this ref (e.g. origin/main, HEAD~5)") | ||
| cmd.Flags().IntVar(&scanPR, "pr", 0, "Incremental mode against a GitHub PR number (requires gh; mutex with --diff-base)") | ||
| cmd.Flags().BoolVar(&scanStaged, "staged", false, "Incremental mode against the staged index vs HEAD (pre-commit hook usage; mutex with --diff-base/--pr)") | ||
| cmd.Flags().StringVar(&scanDiffScope, "diff-scope", "changed_functions", "Diff scope: changed_files, changed_functions, callers") | ||
| } | ||
|
|
||
|
|
@@ -132,6 +134,7 @@ func runScan(cmd *cobra.Command, args []string) { | |
| if decision.Kind == config.ScanKindDiff { | ||
| manifestOpts.base = decision.Base | ||
| manifestOpts.scope = decision.Scope | ||
| manifestOpts.staged = decision.Staged | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium — Architecture · Issue:
Suggestion: Either (a) add a |
||
| } | ||
| manifestPath, err := prepareDiffManifest(repoPath, scanOutput, manifestOpts) | ||
| if err != nil { | ||
|
|
@@ -263,7 +266,7 @@ func finalizeScanMetaIfProject(ctx *projectContext, status string) { | |
| // reflecting the decision so step verbs and finalizeScanMetaIfProject | ||
| // have something to read/update. | ||
| func resolveScanMode(ctx *projectContext, repoPath string) (modeDecision, error) { | ||
| flagsPassed := scanFull || scanIncremental || scanDiffBase != "" || scanPR > 0 | ||
| flagsPassed := scanFull || scanIncremental || scanDiffBase != "" || scanPR > 0 || scanStaged | ||
|
|
||
| // Reuse init's pending decision when no flags override it. | ||
| if !flagsPassed && ctx != nil && ctx.Project != nil { | ||
|
|
@@ -283,6 +286,7 @@ func resolveScanMode(ctx *projectContext, repoPath string) (modeDecision, error) | |
| incremental: scanIncremental, | ||
| diffBase: scanDiffBase, | ||
| pr: scanPR, | ||
| staged: scanStaged, | ||
| scope: scanDiffScope, | ||
| projectName: projectName, | ||
| repoPath: repoPath, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Medium — UX · Python report consumers don't mirror this STAGED special-case
Issue: Nice that the CLI output renders
Diff head: index (staged)for staged manifests. But the samehead_sha = 0000…0000placeholder flows intolibs/openant-core/generate_report.py:109-121(_build_header_subtitle) andlibs/openant-core/export_csv.py:54-65(_format_diff_banner), both of which call_short_sha(head_sha)→00000000. End-users will seeIncremental {repo} · 5e1d2a4..00000000 · …in the HTML report and CSV banner.Suggestion: Mirror this special-case on the Python side — a one-line guard
if base_ref == "STAGED": head = "index"in both_build_header_subtitleand_format_diff_bannerkeeps the report symmetric with the CLI output.