feat: add python and js/ts family support#2
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c094a5c32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .into_iter() | ||
| .map(|change| DisplayStat { |
There was a problem hiding this comment.
When a file moves between supported languages, filter_by_langs now keeps the change for either selected language, but this TestFilterMode::All branch still emits the raw diff totals. In a rename like tests/test_mod.py => src/lib.rs, --lang py --no-test-filter will include the Rust insertions and --lang rs --no-test-filter will include the Python deletions, so the reported language scope is wrong for any cross-language rename when test filtering is disabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f525a48. --no-test-filter now goes through the same side-aware stats builder instead of emitting raw diff totals, so py -> rs renames only contribute deletions for --lang py and only insertions for --lang rs. Added no_test_filter_splits_cross_language_rename_by_selected_language to lock the end-to-end behavior.
| let (added, deleted) = match (old_language, new_language) { | ||
| (Some(old), Some(new)) if old != new => build_counts_for_cross_language_change( |
There was a problem hiding this comment.
This only takes the side-splitting path when both old and new paths are recognized languages. A rename such as README.md => app/main.py or image.bin => src/lib.rs still passes the language filter because one side matches, but the _ arm then runs the Python/Rust backend over both sides. That misattributes deleted non-Python/non-Rust lines to the selected language, and with a binary old file it can fail outright when show_* returns non-UTF8. Renames where only one side is supported need the same per-side handling as other cross-language changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f525a48. Mixed-language handling now applies whenever old_path and new_path resolve to different language states, including supported↔unsupported renames, so only the selected side contributes to the stats. Added non_test_filter_splits_supported_to_unsupported_rename_by_selected_language to cover the README.md -> src/lib.rs path.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f525a4860f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let patch_output = git.diff_patch(&selection.git_diff_args())?; | ||
| let patch = parse_patch(&patch_output)?; | ||
| let patch_map = patch | ||
| .files | ||
| .into_iter() | ||
| .map(|file| (file.path.clone(), file)) | ||
| .collect::<HashMap<_, _>>(); | ||
| let endpoints = selection.endpoints(git)?; | ||
| let whole_test_paths = build_whole_test_paths(git, endpoints.as_ref(), langs)?; |
There was a problem hiding this comment.
--no-test-filter before parsing sources
When mode is All (--no-test-filter), this path still parses the diff, builds whole-test indexes, and later calls split_untracked / split_patch for Rust and Python files. In practice that means git diff-stat --no-test-filter can now fail on non-UTF8 or temporarily invalid .rs / .py sources and does a lot of unnecessary repo-wide parsing, even though the README/design for this flag says it should report full-file stats directly. The previous implementation returned raw change.added / change.deleted here, so this is a regression in the flag's documented behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fed250a. --no-test-filter now short-circuits before patch parsing and source loading, so unfiltered runs no longer depend on Rust/Python source decoding or test-split parsing. Added no_test_filter_does_not_parse_invalid_rust_sources to lock the regression case.
| for change in changes { | ||
| if change.added + change.deleted == 0 { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
--no-test-filter output
This unconditional skip removes pure renames before we reach TestFilterMode::All. git diff --numstat --find-renames still reports rename-only changes as 0 0 old => new, and the old --no-test-filter path rendered those entries, so a commit/range that only renames a.js to b.js now prints 0 files changed instead of showing the rename line. That regresses parity with git diff --stat for rename-only changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fed250a. The unfiltered fast path now preserves rename-only entries instead of dropping 0/0 changes, so --no-test-filter keeps parity with git diff --stat for pure renames. Added no_test_filter_keeps_rename_only_entries as the end-to-end regression test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fed250a1c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for change in changes { | ||
| if change.added + change.deleted == 0 { | ||
| continue; |
There was a problem hiding this comment.
For rename-only Python/JS changes in --test or the default --no-test mode (for example tests/test_app.py -> tests/test_math.py or web/app.spec.tsx -> web/app.test.tsx), git diff --numstat --find-renames reports 0\t0, so this unconditional skip drops the file before the new whole-file path classification runs. In those commits/ranges the command now prints 0 files changed even though a supported file moved, so the newly added Python/JS support loses rename parity whenever there are no line edits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9c4b68b. The test-filter path no longer drops 0/0 changes before classification, so rename-only test files are preserved in filtered output. Added CLI regressions for both Python --test and TSX default non-test cases to lock this behavior down.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c4b68bcc1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if change.added + change.deleted == 0 { | ||
| if change.old_path != change.new_path { | ||
| stats.push(DisplayStat { | ||
| path: change.path.clone(), | ||
| added: 0, |
There was a problem hiding this comment.
--test output
When change.added + change.deleted == 0, this branch emits every rename before any whole-file/path or region-based test classification runs. In practice, a pure rename like src/app.py => src/util.py will still be reported for git diff-stat --lang py --test as 1 files changed, even though neither side contributes any test code. That makes --test over-report files for rename-only non-test changes while ordinary non-test edits are correctly filtered out.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
这个问题我看过，结论是当前先不修。
原因是这里涉及 rename-only 的 0/0 变更在 --test / --non-test 下到底该不该被视为“属于某一类代码”的语义定义。要把它完全做对，不能只补一个局部判断，还要把 Rust/Python 这类可能混合 test/non-test region 的文件一起纳入考虑，复杂度会明显上升。
这次我更倾向于保留现在的简单策略：filtered mode 下对 rename-only 继续保留条目，优先保持和 git diff --stat 的 rename 展示一致性，而不对零行变更做更细的 test/non-test 推断。后面如果这个边角行为在真实使用里造成困扰，再单独开 issue 讨论更完整的语义。
|
@codex review ，用中文 |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b811be64ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if change.added + change.deleted == 0 { | ||
| if change.old_path != change.new_path { | ||
| stats.push(DisplayStat { | ||
| path: change.path.clone(), | ||
| added: 0, | ||
| deleted: 0, | ||
| }); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
This zero-line fast path keeps every pure rename without checking whether the file belongs to the selected test scope. In practice, git diff-stat --test will still list renames of non-test files (for example src/app.js -> src/main.js), and the default non-test mode will also list pure renames of test-only files, because the language-specific classification below never runs. That makes rename-only commits produce inaccurate filtered reports.
Useful? React with 👍 / 👎.
1 participant
Summary
--langto all supported languages instead of a hardcoded subsetjs,ts,jsx,tsx,cjs,mjs--testand--no-testclassify old and new sides with the correct language backendTesting