feat(pr diff): add --exclude flag to filter files from diff output#12655
feat(pr diff): add --exclude flag to filter files from diff output#12655yuvrajangadsingh wants to merge 4 commits intocli:trunkfrom
Conversation
186d5d5 to
bc15e7d
Compare
|
@BagToad @babakks hey, just wanted to check if this is something the team would be interested in — adds an --exclude flag to gh pr diff so you can filter out noisy files (like lockfiles, snapshots, generated code). pretty common ask based on the issues i've seen. let me know if there's anything to adjust! |
aa530d3 to
e147705
Compare
|
gentle bump on this one — would love to get some eyes on it when someone has a chance. happy to make changes if anything needs adjusting. |
e147705 to
be8446c
Compare
BagToad
left a comment
There was a problem hiding this comment.
Nice implementation — the exclude logic is clean and well-tested.
One change needed: the diff header regex is duplicated. changedFilesNames (line ~310) already has an inline regex that's nearly identical to your new diffHeaderRegexp:
// existing (changedFilesNames):
pattern := regexp.MustCompile(`(?:^|\n)diff\s--git.*\s(["']?)b/(.*)`)
// new (extractFileName):
var diffHeaderRegexp = regexp.MustCompile(`diff\s--git.*\s(["']?)b/(.*)`)Could you share the compiled regex between both and ideally refactor changedFilesNames to use your splitDiffSections/extractFileName helpers? That would reduce duplication and make the code easier to maintain.
If this is too painful, let me know — it's kind of a nit so I'm happy to tackle it in a follow-up if needed instead.
|
Also note, we need to fix the linter checks. |
be8446c to
fc3b9ef
Compare
|
@BagToad fixed both — shared the compiled diffHeaderRegexp between changedFilesNames and extractFileName (removed the inline duplicate), and fixed the gofmt spacing. rebased on trunk too. |
fc3b9ef to
40ac081
Compare
babakks
left a comment
There was a problem hiding this comment.
Thanks for the PR, @yuvrajangadsingh! 🙏
Just commented some thoughts, but the main one is the last (about using path instead of filepath).
| // This is kind of a gnarly regex. We're looking lines of the format: | ||
| // diff --git a/9114-triage b/9114-triage | ||
| // diff --git "a/hello-\360\237\230\200-world" "b/hello-\360\237\230\200-world" | ||
| // | ||
| // From these lines we would look to extract: | ||
| // 9114-triage | ||
| // "hello-\360\237\230\200-world" | ||
| // | ||
| // Note that the b/ is removed but in the second case the preceeding quote remains. | ||
| // This is important for how git handles filenames that would be quoted with core.quotePath. | ||
| // https://git-scm.com/docs/git-config#Documentation/git-config.txt-corequotePath | ||
| // | ||
| // Thus we capture the quote if it exists, and everything that follows the b/ | ||
| // We then concatenate those two capture groups together which for the examples above would be: | ||
| // `` + 9114-triage | ||
| // `"`` + hello-\360\237\230\200-world" | ||
| // | ||
| // Where I'm using the `` to indicate a string to avoid confusion with the " character. | ||
| pattern := regexp.MustCompile(`(?:^|\n)diff\s--git.*\s(["]?)b/(.*)`) | ||
| matches := pattern.FindAllStringSubmatch(string(diff), -1) | ||
| matches := diffHeaderRegexp.FindAllStringSubmatch(string(diff), -1) |
There was a problem hiding this comment.
thought: we should really think about using a reliable third-party for this stuff. 🤷
There was a problem hiding this comment.
💯 agree. Felt this pain with gh agent as well.
There was a problem hiding this comment.
agreed, would be nice to have. out of scope for this PR though
16fd9ad to
f2ac428
Compare
Add a new --exclude (-e) flag to gh pr diff that allows users to exclude files matching glob patterns from the diff output. This is useful for filtering out auto-generated files, vendor directories, or other noise when reviewing pull requests. Supports standard glob patterns and can be specified multiple times. Patterns match against both the full path and basename. Closes cli#8739
- add usage examples to --help output - use path.Match instead of filepath.Match for OS-consistent behavior - rename patterns to excludePatterns for clarity - simplify splitDiffSections using strings.Split
f2ac428 to
a331ef7
Compare
Adds a
--exclude(-e) flag togh pr diffthat lets you filter out files matching glob patterns from the diff output.Example usage:
Patterns are matched against both the full file path and the basename, so
--exclude '*.yml'will matchdir/file.yml.Uses Go's
filepath.Matchfor glob support — no new dependencies.Closes #8739