Fix: direct commits still misclassified after #337 (compare mode) - #339
Fix: direct commits still misclassified after #337 (compare mode)#339miroslavpojer wants to merge 5 commits into
Conversation
|
Warning Review limit reachedNext included review available in 11 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes compare-mode misclassification where commits that belong to merged PRs can still leak into release notes as “direct commits,” especially when PR numbers aren’t discoverable via commit message text.
Changes:
- Expanded PR-number extraction to include bare leading
#Nsubjects and added tests for positive/negative cases. - Added a capped “commit → associated PRs” fallback (
commit.get_pulls()) to resolve merged PR ownership when commit messages never mention the PR number. - Added debug logging for direct-commit registration and compare-mode PR/commit classification, plus updated unit-test fixtures impacted by the new logging access.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| release_notes_generator/data/miner.py | Extends PR-number detection and adds an association-endpoint fallback + diagnostics for compare-mode classification. |
| release_notes_generator/record/factory/default_record_factory.py | Adds debug logging when registering direct commits and when matching PR commit SHAs. |
| tests/unit/release_notes_generator/data/test_miner.py | Adds unit coverage for bare #N extraction and commit→PR association fallback behavior/cap. |
| tests/unit/release_notes_generator/record/factory/test_default_record_factory.py | Fixes fixtures to populate .commit.message to satisfy new debug logging. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…and adding tests for edge cases
…rove logging for commit SHAs
…ove error logging
tmikula-dev
left a comment
tmikula-dev
left a comment
There was a problem hiding this comment.
The logic looks correct. I just wrote two comments/questions to think about. In this PR there is too much comments and notes IMO. Can be personal opinion, still approving.
|
|
||
| # dev note: cap on how many matched commit SHAs are logged at debug level, to keep verbose logs | ||
| # readable for PRs with many commits. | ||
| _MAX_LOGGED_COMMIT_SHAS = 50 |
There was a problem hiding this comment.
Verbose logging should IMO not be limited. If there is a case, when a 50 commits are mined, it is already not readable. Now you delete the information that could be useful.
| # dev note: 3rd alternative catches commits whose subject leads with a bare "#N" reference | ||
| # (e.g. "#1403 Fix thing"), a message style left as-is by some merge strategies (e.g. rebase-merge) | ||
| # that don't append GitHub's "(#N)"/"Merge pull request #N" boilerplate. Without it, such PRs are | ||
| # never looked up at all, so their commits can't be excluded as duplicates of the PR. | ||
| # Unlike _PR_MERGE_ARTIFACT_RE, this is only a candidate to try fetching - #N is a common commit | ||
| # convention for referencing an issue and is not proof the commit belongs to a real merged PR, so it | ||
| # must not by itself exclude a commit (see the SHA-based check in _handle_compare_mode). |
There was a problem hiding this comment.
This is just the beautifying, but crucial for clean code for easy readability and review process. I do not think that code should be place, where you explain a logic in 7 rows. In this PR there is many many generated dev notes:. If every logic row needs two extra comment ones to explain it. It has a bad naming or is just generated extra stuff, that is not adding any extra value.
The point is a lot of added stuff in this PR starts to overwhelm the logic itself.
Problem
Retesting the fix from #337 against
absa-group/ursa-metalake(v1.7.1...v1.8.1, compare mode)showed commits still leaking into the release notes as stand-alone "direct commits" even though
they belonged to a merged PR. Two distinct gaps were found:
_PR_NUMBER_REwas too narrow. It only matched(#N)(squash-merge style) andMerge pull request #N(merge-commit style). Commit subjects like#1403 Investigate and fix omd dockerfile certificate issue(a bare leading#N, no parens,as left behind by e.g. rebase-merge) were never recognized, so
_extract_pr_numbers_from_commitsnever found PR #1403 in the first place — the PR was never even fetched via
get_pull(), so itscommit could never be excluded.
A PR's commits can reference a completely different number than the PR itself. Commit
d1c99fe("Hotfix v1.8.0 -> 1.8.1") belongs to PR #1430, but none of PR #1430's commits mention#1430anywhere — they reference unrelated numbers instead (e.g.#1427). No regex on committext can ever discover such a PR, because the PR number literally doesn't appear in any of its
commit messages.
Fix
release_notes_generator/data/miner.py:_PR_NUMBER_REwith a third alternative for a bare leading#Nreference(
^#(\d+)\b), so PRs like #1403 are looked up instead of silently skipped._handle_compare_mode: for each commit still classified as a"direct commit" candidate after the message-based pass, call GitHub's
commit → associated-PRs endpoint (
commit.get_pulls()). If it resolves to a merged PR, thatPR's full commit set (
get_commits()+merge_commit_sha) is registered and the commit isexcluded — regardless of what its message says.
_MAX_DIRECT_COMMIT_PR_LOOKUPS(200) to bound worst-case extra API callsfor repositories/ranges with genuinely many direct commits; skips with a debug log beyond that.
_register_pr_commit_shasto avoidduplicating it between the primary and fallback passes.
per-PR commit SHAs, exclusion/classification reason per commit) to make future retests
diagnosable from
INPUT_VERBOSE=trueoutput alone.release_notes_generator/record/factory/default_record_factory.py: added a debug log line when acommit is registered as a direct commit, for the same diagnostic reason (since-time mode).
Testing
test_miner.py:#NPR-number extraction (positive and negative case)._MAX_DIRECT_COMMIT_PR_LOOKUPS.test_default_record_factory.pyfixtures whose mocked commits didn't set.commit.message, now touched by the new debug log.absa-group/ursa-metalakev1.7.1...v1.8.1:direct-commit count went 8 → 3 (regex fix) → 0 (fallback fix); PR count went 5 → 8, matching the
actual repository state.
_handle_since_time_mode) was not affected by this bug — it fetches all closedPRs unconditionally rather than relying on commit-message extraction — and was not locally
re-tested end-to-end for this change, only covered by existing unit tests.
Release Notes
#Nprefix (no parentheses) were misclassified asdirect commits in compare mode.
were misclassified as direct commits in compare mode.
commits in compare mode.
registration, to aid future diagnosis.