-
Notifications
You must be signed in to change notification settings - Fork 9
Add git-aware diff linting: skillsaw lint --since REF
#401
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: main
Are you sure you want to change the base?
Changes from all commits
5df00ee
226c1a5
b869de0
e835d77
bfc4326
b07f5a5
28ce991
5e54ef6
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 | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -45,6 +45,14 @@ def _run_lint(args): | |||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| sys.exit(1) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| if args.since and args.no_baseline: | ||||||||||||||||||||||||||||||||||||||||||||||||
| print( | ||||||||||||||||||||||||||||||||||||||||||||||||
| "Error: --since and --no-baseline cannot be combined " | ||||||||||||||||||||||||||||||||||||||||||||||||
| "(--since builds its own ephemeral baseline from git)", | ||||||||||||||||||||||||||||||||||||||||||||||||
| file=sys.stderr, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| sys.exit(1) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| output_formats = {} | ||||||||||||||||||||||||||||||||||||||||||||||||
| for spec in args.outputs: | ||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -111,8 +119,40 @@ def _run_lint(args): | |||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||
| fail_level = config.effective_fail_level() | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| rule_ids = set(args.rule_ids) if args.rule_ids else None | ||||||||||||||||||||||||||||||||||||||||||||||||
| skip_rule_ids = set(args.skip_rule_ids) if args.skip_rule_ids else None | ||||||||||||||||||||||||||||||||||||||||||||||||
| if rule_ids and skip_rule_ids: | ||||||||||||||||||||||||||||||||||||||||||||||||
| print("Error: --rule and --skip-rule cannot be combined", file=sys.stderr) | ||||||||||||||||||||||||||||||||||||||||||||||||
| sys.exit(1) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| baseline = None | ||||||||||||||||||||||||||||||||||||||||||||||||
| if not args.no_baseline: | ||||||||||||||||||||||||||||||||||||||||||||||||
| if args.since: | ||||||||||||||||||||||||||||||||||||||||||||||||
| # --since builds an ephemeral baseline from the merge-base of HEAD | ||||||||||||||||||||||||||||||||||||||||||||||||
| # and the given ref; it takes precedence over any committed | ||||||||||||||||||||||||||||||||||||||||||||||||
| # .skillsaw-baseline.json. | ||||||||||||||||||||||||||||||||||||||||||||||||
| from ..git_baseline import GitBaselineError, build_git_baseline, repo_toplevel | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||
| repo_root = repo_toplevel(paths[0]) | ||||||||||||||||||||||||||||||||||||||||||||||||
| baseline, merge_base = build_git_baseline( | ||||||||||||||||||||||||||||||||||||||||||||||||
| repo_root, | ||||||||||||||||||||||||||||||||||||||||||||||||
| args.since, | ||||||||||||||||||||||||||||||||||||||||||||||||
| config, | ||||||||||||||||||||||||||||||||||||||||||||||||
| paths, | ||||||||||||||||||||||||||||||||||||||||||||||||
| cli_version, | ||||||||||||||||||||||||||||||||||||||||||||||||
| repo_types=override_types, | ||||||||||||||||||||||||||||||||||||||||||||||||
| rule_ids=rule_ids, | ||||||||||||||||||||||||||||||||||||||||||||||||
| skip_rule_ids=skip_rule_ids, | ||||||||||||||||||||||||||||||||||||||||||||||||
| no_custom_rules=args.no_custom_rules, | ||||||||||||||||||||||||||||||||||||||||||||||||
| no_plugins=args.no_plugins, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+137
to
+148
Contributor
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. Pass
Suggested change
Collaborator
Author
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. Done in b869de0 — the call site now passes |
||||||||||||||||||||||||||||||||||||||||||||||||
| except GitBaselineError as e: | ||||||||||||||||||||||||||||||||||||||||||||||||
| print(f"Error: {e}", file=sys.stderr) | ||||||||||||||||||||||||||||||||||||||||||||||||
| sys.exit(1) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| if args.fmt == "text": | ||||||||||||||||||||||||||||||||||||||||||||||||
| print(f"Comparing against merge-base {merge_base[:12]} (--since {args.since})\n") | ||||||||||||||||||||||||||||||||||||||||||||||||
| elif not args.no_baseline: | ||||||||||||||||||||||||||||||||||||||||||||||||
| from ..baseline import find_baseline, load_baseline | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| baseline_path = find_baseline(config.config_dir or paths[0]) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -128,12 +168,6 @@ def _run_lint(args): | |||||||||||||||||||||||||||||||||||||||||||||||
| except (ValueError, OSError) as e: | ||||||||||||||||||||||||||||||||||||||||||||||||
| print(f"Warning: Failed to load baseline: {e}", file=sys.stderr) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| rule_ids = set(args.rule_ids) if args.rule_ids else None | ||||||||||||||||||||||||||||||||||||||||||||||||
| skip_rule_ids = set(args.skip_rule_ids) if args.skip_rule_ids else None | ||||||||||||||||||||||||||||||||||||||||||||||||
| if rule_ids and skip_rule_ids: | ||||||||||||||||||||||||||||||||||||||||||||||||
| print("Error: --rule and --skip-rule cannot be combined", file=sys.stderr) | ||||||||||||||||||||||||||||||||||||||||||||||||
| sys.exit(1) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| lint_started = time.perf_counter() | ||||||||||||||||||||||||||||||||||||||||||||||||
| all_violations = [] | ||||||||||||||||||||||||||||||||||||||||||||||||
| all_rules = [] | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -188,16 +222,22 @@ def _run_lint(args): | |||||||||||||||||||||||||||||||||||||||||||||||
| if baseline and args.fmt == "text": | ||||||||||||||||||||||||||||||||||||||||||||||||
| stale = linter.stale_baseline_entries | ||||||||||||||||||||||||||||||||||||||||||||||||
| if stale: | ||||||||||||||||||||||||||||||||||||||||||||||||
| print( | ||||||||||||||||||||||||||||||||||||||||||||||||
| f"Baseline: {len(stale)} stale" | ||||||||||||||||||||||||||||||||||||||||||||||||
| f" {'entry' if len(stale) == 1 else 'entries'}" | ||||||||||||||||||||||||||||||||||||||||||||||||
| f" (violations resolved since baseline was set)" | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| if args.since: | ||||||||||||||||||||||||||||||||||||||||||||||||
| print(f"Baseline: {len(stale)} violation(s) fixed since {args.since}") | ||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||
| print( | ||||||||||||||||||||||||||||||||||||||||||||||||
| f"Baseline: {len(stale)} stale" | ||||||||||||||||||||||||||||||||||||||||||||||||
| f" {'entry' if len(stale) == 1 else 'entries'}" | ||||||||||||||||||||||||||||||||||||||||||||||||
| f" (violations resolved since baseline was set)" | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
| if args.verbose: | ||||||||||||||||||||||||||||||||||||||||||||||||
| for entry in stale: | ||||||||||||||||||||||||||||||||||||||||||||||||
| location = f" [{entry.file_path}]" if entry.file_path else "" | ||||||||||||||||||||||||||||||||||||||||||||||||
| print(f" - {entry.rule_id}{location}: {entry.message}") | ||||||||||||||||||||||||||||||||||||||||||||||||
| print(" Run `skillsaw baseline` to update.\n") | ||||||||||||||||||||||||||||||||||||||||||||||||
| if args.since: | ||||||||||||||||||||||||||||||||||||||||||||||||
| print() | ||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||
| print(" Run `skillsaw baseline` to update.\n") | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| merged_context = _build_merged_context(contexts) | ||||||||||||||||||||||||||||||||||||||||||||||||
| unique_rules = _dedup_rules(all_rules) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Define
BASE_REFbefore using it.BASE_REFis never set in this snippet, so the fallback fetch command fails if someone copies it verbatim. Inline the GitHub expression or define the variable first.💡 Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents