ci: add feature audit checks#1193
Conversation
|
Caution Review failedThe pull request is closed. """ WalkthroughThis update introduces a feature version audit system integrated into the GitHub Actions workflow. It adds three new jobs for auditing feature versions on releases and pull requests, generating markdown reports, and posting results as PR comments. A new Python script implements the audit logic, and the release process is updated to include audit results in release notes. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant feature_version_audit.py
participant Artifact Store
participant PR/Release
GitHub Actions->>feature_version_audit.py: Run audit script (on release or PR)
feature_version_audit.py->>Artifact Store: Upload markdown report
GitHub Actions->>PR/Release: (On PR) Post comment with audit report
GitHub Actions->>PR/Release: (On release) Append audit report to release notes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
tools/feature_version_audit.py (3)
323-323: Rename unused loop variableThe loop control variable
statusis not used within the loop body.- for status, f in changes: + for _, f in changes:
514-514: Rename unused loop variableThe loop control variable
fnameis not used within the loop body.- for fname, info in feature_actions.items(): + for _, info in feature_actions.items():
568-570: Remove unused global declarationsThese global declarations are unnecessary since the variables are only read, not modified.
def main(): - global RELEASE_TAG, FEATURES_DIR, SHADER_TYPES parser = argparse.ArgumentParser(description="Feature version audit for Skyrim Community Shaders.").github/workflows/build.yaml (2)
615-627: Add GITHUB_TOKEN environment variable for gh CLIWhile the gh CLI typically uses the default token, it's better to be explicit.
- name: Generate combined release notes id: combined_notes shell: bash + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | # Generate GitHub release notes gh release view ${{ github.ref_name }} --json body -q .body > auto-release-notes.md || echo '' > auto-release-notes.md
517-591: Consider error handling strategy for productionThe audit jobs currently have
continue-on-error: truewhich is appropriate for testing but should be reconsidered before merging to production. Failed audits might indicate important issues that shouldn't be silently ignored.For production readiness, consider:
- Remove
continue-on-erroronce the feature is stable- Add notifications or create issues when audits fail
- Make the audit results required status checks for PRs
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yaml(2 hunks)tools/feature_version_audit.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
tools/feature_version_audit.py
323-323: Loop control variable status not used within loop body
Rename unused status to _status
(B007)
374-374: Function definition does not bind loop variable is_attention
(B023)
429-429: Function definition does not bind loop variable fa
(B023)
450-450: Function definition does not bind loop variable missing
(B023)
477-477: Function definition does not bind loop variable missing
(B023)
514-514: Loop control variable fname not used within loop body
Rename unused fname to _fname
(B007)
655-657: Use a single if statement instead of nested if statements
(SIM102)
🪛 Flake8 (7.2.0)
tools/feature_version_audit.py
[error] 47-47: expected 2 blank lines, found 1
(E302)
[error] 51-51: expected 2 blank lines, found 1
(E302)
[error] 54-54: expected 2 blank lines, found 1
(E302)
[error] 61-61: expected 2 blank lines, found 1
(E302)
[error] 73-73: expected 2 blank lines, found 1
(E302)
[error] 84-84: expected 2 blank lines, found 1
(E302)
[error] 101-101: expected 2 blank lines, found 1
(E302)
[error] 111-111: expected 2 blank lines, found 1
(E302)
[error] 126-126: expected 2 blank lines, found 1
(E302)
[error] 136-136: expected 2 blank lines, found 1
(E302)
[error] 178-178: expected 2 blank lines, found 1
(E302)
[error] 214-214: expected 2 blank lines, found 1
(E302)
[error] 223-223: expected 1 blank line before a nested definition, found 0
(E306)
[error] 230-230: expected 2 blank lines, found 1
(E302)
[error] 245-245: expected 2 blank lines, found 1
(E302)
[error] 256-256: expected 2 blank lines, found 1
(E302)
[error] 270-270: expected 2 blank lines, found 1
(E302)
[error] 298-298: expected 1 blank line before a nested definition, found 0
(E306)
[error] 373-373: expected 1 blank line before a nested definition, found 0
(E306)
[error] 394-394: expected 2 blank lines, found 1
(E302)
[error] 403-403: expected 2 blank lines, found 1
(E302)
[error] 409-409: missing whitespace after ','
(E231)
[error] 409-409: missing whitespace after ','
(E231)
[error] 409-409: missing whitespace after ','
(E231)
[error] 410-410: missing whitespace after ','
(E231)
[error] 410-410: missing whitespace after ','
(E231)
[error] 410-410: missing whitespace after ','
(E231)
[error] 414-414: expected 2 blank lines, found 1
(E302)
[error] 423-423: expected 2 blank lines, found 1
(E302)
[error] 433-433: expected 2 blank lines, found 1
(E302)
[error] 449-449: expected 1 blank line before a nested definition, found 0
(E306)
[error] 458-458: expected 2 blank lines, found 1
(E302)
[error] 476-476: expected 1 blank line before a nested definition, found 0
(E306)
[error] 486-486: expected 2 blank lines, found 1
(E302)
[error] 512-512: expected 2 blank lines, found 1
(E302)
[error] 529-529: expected 2 blank lines, found 1
(E302)
[error] 568-568: expected 2 blank lines, found 1
(E302)
[error] 569-569: global RELEASE_TAG is unused: name is never assigned in scope
(F824)
[error] 569-569: global FEATURES_DIR is unused: name is never assigned in scope
(F824)
[error] 569-569: global SHADER_TYPES is unused: name is never assigned in scope
(F824)
[error] 611-611: expected 1 blank line before a nested definition, found 0
(E306)
[error] 669-669: expected 2 blank lines after class or function definition, found 1
(E305)
🪛 Pylint (3.3.7)
tools/feature_version_audit.py
[refactor] 178-178: Too many local variables (16/15)
(R0914)
[refactor] 263-268: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 270-270: Too many local variables (44/15)
(R0914)
[refactor] 365-365: Too many boolean expressions in if statement (6/5)
(R0916)
[refactor] 270-270: Too many branches (26/12)
(R0912)
[refactor] 270-270: Too many statements (92/50)
(R0915)
[refactor] 433-433: Too many local variables (17/15)
(R0914)
[refactor] 486-486: Too many local variables (16/15)
(R0914)
[refactor] 529-529: Too many arguments (11/5)
(R0913)
[refactor] 529-529: Too many positional arguments (11/5)
(R0917)
[refactor] 529-529: Too many local variables (19/15)
(R0914)
[refactor] 568-568: Too many local variables (25/15)
(R0914)
[refactor] 568-568: Too many branches (20/12)
(R0912)
[refactor] 568-568: Too many statements (65/50)
(R0915)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tools/feature_version_audit.py (4)
373-374: Fix function closure bug that captures loop variableThe
boldfunction captures the loop variableis_attention, which will always have the value from the last iteration when the function is called later.- def bold(val): - return f"**{val}**" if is_attention and val != '' and val != '-' else val + def bold(val, is_attention=is_attention): + return f"**{val}**" if is_attention and val != '' and val != '-' else val
428-430: Fix function closure bug in format_feature_tableThe
boldfunction captures the loop variablefa, causing all calls to use the last iteration's value.- def bold(val, is_attention=fa['is_attention']): - return f"**{val}**" if is_attention and val != '' and val != '-' else val - lines.append(f"| {bold(fa['name'])} | {bold(fa['prior_ver_str'])} | {bold(fa['proposed_ver_str'])} | {bold(str(fa['needs_bump']))} | {bold(fa['change_types'])} | {bold(fa['note'])} | {fa['commit_link']} |") + is_attention = fa['is_attention'] + def bold(val): + return f"**{val}**" if is_attention and val != '' and val != '-' else val + lines.append(f"| {bold(fa['name'])} | {bold(fa['prior_ver_str'])} | {bold(fa['proposed_ver_str'])} | {bold(str(fa['needs_bump']))} | {bold(fa['change_types'])} | {bold(fa['note'])} | {fa['commit_link']} |")
449-455: Fix function closure bug in format_new_features_tableThe
boldmetafunction captures the loop variablemissing, which can cause incorrect formatting.- def boldmeta(val, missing=missing): - return f"**{val}**" if missing and val != '' and val != '-' else val + def boldmeta(val, is_missing): + return f"**{val}**" if is_missing and val != '' and val != '-' else val nexus_link = f"[Nexus]({meta['mod_link']})" if meta and meta['mod_link'] else ("**Missing metadata**" if not meta else "") author = get_commit_author(commit) if commit else None author_str = f" ({author})" if author else "" commit_link = f"[link](https://github.com/doodlum/skyrim-community-shaders/commit/{commit}){author_str}" if commit else "" - lines.append(f"| {boldmeta(name)} | {boldmeta(ver)} | {nexus_link} | {commit_link} |") + lines.append(f"| {boldmeta(name, missing)} | {boldmeta(ver, missing)} | {nexus_link} | {commit_link} |")
476-481: Fix function closure bug in format_metadata_summaryThe
boldmetafunction captures the loop variablemissing.- def boldmeta(val, missing=missing): - return f"**{val}**" if missing else val + def boldmeta(val, is_missing): + return f"**{val}**" if is_missing else val link = f"[Nexus]({info['mod_link']})" if info['mod_link'] else "" desc = info['description'][:80] + ("..." if len(info['description']) > 80 else "") keys = ", ".join(info['key_features'][:3]) + (", ..." if len(info['key_features']) > 3 else "") if info['key_features'] else "" - lines.append(f"| {boldmeta(info['name'])} | {info['is_core']} | {link} | {desc} | {keys} |") + lines.append(f"| {boldmeta(info['name'], missing)} | {info['is_core']} | {link} | {desc} | {keys} |")
🧹 Nitpick comments (4)
tools/feature_version_audit.py (4)
323-323: Rename unused loop variableThe loop variable
statusis not used within the loop body.- for status, f in changes: + for _status, f in changes:
514-514: Rename unused loop variableThe loop variable
fnameis not used within the loop body.- for fname, info in feature_actions.items(): + for _fname, info in feature_actions.items():
654-656: Simplify nested if statementsThese nested if statements can be combined into a single condition.
- if args.output or args.ci or args.fail_on_actionable: - # Only print actionable items if not in audit/report mode (i.e., no --output) - if not args.output: + if (args.output or args.ci or args.fail_on_actionable) and not args.output: + # Only print actionable items if not in audit/report mode (i.e., no --output)
265-268: Remove unnecessary elif after returnSince the function returns in the if block, the elif can be changed to if for cleaner code.
if has_feat: return (major, minor + 1, 0) - elif has_fix: + if has_fix: return (major, minor, patch + 1) - else: - return prior_version + return prior_version
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yaml(2 hunks)tools/feature_version_audit.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build.yaml
🧰 Additional context used
🪛 Ruff (0.11.9)
tools/feature_version_audit.py
323-323: Loop control variable status not used within loop body
Rename unused status to _status
(B007)
374-374: Function definition does not bind loop variable is_attention
(B023)
514-514: Loop control variable fname not used within loop body
Rename unused fname to _fname
(B007)
654-656: Use a single if statement instead of nested if statements
(SIM102)
🪛 Flake8 (7.2.0)
tools/feature_version_audit.py
[error] 47-47: expected 2 blank lines, found 1
(E302)
[error] 51-51: expected 2 blank lines, found 1
(E302)
[error] 54-54: expected 2 blank lines, found 1
(E302)
[error] 61-61: expected 2 blank lines, found 1
(E302)
[error] 73-73: expected 2 blank lines, found 1
(E302)
[error] 84-84: expected 2 blank lines, found 1
(E302)
[error] 101-101: expected 2 blank lines, found 1
(E302)
[error] 111-111: expected 2 blank lines, found 1
(E302)
[error] 126-126: expected 2 blank lines, found 1
(E302)
[error] 136-136: expected 2 blank lines, found 1
(E302)
[error] 178-178: expected 2 blank lines, found 1
(E302)
[error] 214-214: expected 2 blank lines, found 1
(E302)
[error] 223-223: expected 1 blank line before a nested definition, found 0
(E306)
[error] 230-230: expected 2 blank lines, found 1
(E302)
[error] 245-245: expected 2 blank lines, found 1
(E302)
[error] 256-256: expected 2 blank lines, found 1
(E302)
[error] 270-270: expected 2 blank lines, found 1
(E302)
[error] 298-298: expected 1 blank line before a nested definition, found 0
(E306)
[error] 373-373: expected 1 blank line before a nested definition, found 0
(E306)
[error] 394-394: expected 2 blank lines, found 1
(E302)
[error] 403-403: expected 2 blank lines, found 1
(E302)
[error] 409-409: missing whitespace after ','
(E231)
[error] 409-409: missing whitespace after ','
(E231)
[error] 409-409: missing whitespace after ','
(E231)
[error] 410-410: missing whitespace after ','
(E231)
[error] 410-410: missing whitespace after ','
(E231)
[error] 410-410: missing whitespace after ','
(E231)
[error] 414-414: expected 2 blank lines, found 1
(E302)
[error] 423-423: expected 2 blank lines, found 1
(E302)
[error] 433-433: expected 2 blank lines, found 1
(E302)
[error] 449-449: expected 1 blank line before a nested definition, found 0
(E306)
[error] 458-458: expected 2 blank lines, found 1
(E302)
[error] 476-476: expected 1 blank line before a nested definition, found 0
(E306)
[error] 486-486: expected 2 blank lines, found 1
(E302)
[error] 512-512: expected 2 blank lines, found 1
(E302)
[error] 529-529: expected 2 blank lines, found 1
(E302)
[error] 568-568: expected 2 blank lines, found 1
(E302)
[error] 569-569: global RELEASE_TAG is unused: name is never assigned in scope
(F824)
[error] 569-569: global FEATURES_DIR is unused: name is never assigned in scope
(F824)
[error] 569-569: global SHADER_TYPES is unused: name is never assigned in scope
(F824)
[error] 610-610: expected 1 blank line before a nested definition, found 0
(E306)
[error] 668-668: expected 2 blank lines after class or function definition, found 1
(E305)
🪛 Pylint (3.3.7)
tools/feature_version_audit.py
[refactor] 178-178: Too many local variables (16/15)
(R0914)
[refactor] 263-268: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 270-270: Too many local variables (44/15)
(R0914)
[refactor] 365-365: Too many boolean expressions in if statement (6/5)
(R0916)
[refactor] 270-270: Too many branches (26/12)
(R0912)
[refactor] 270-270: Too many statements (92/50)
(R0915)
[refactor] 433-433: Too many local variables (17/15)
(R0914)
[refactor] 486-486: Too many local variables (16/15)
(R0914)
[refactor] 529-529: Too many arguments (11/5)
(R0913)
[refactor] 529-529: Too many positional arguments (11/5)
(R0917)
[refactor] 529-529: Too many local variables (19/15)
(R0914)
[refactor] 568-568: Too many local variables (24/15)
(R0914)
[refactor] 568-568: Too many branches (20/12)
(R0912)
[refactor] 568-568: Too many statements (64/50)
(R0915)
| return output | ||
|
|
||
| def main(): | ||
| global RELEASE_TAG, FEATURES_DIR, SHADER_TYPES |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary global declarations
These global declarations are not needed since the variables are never reassigned within the function scope.
def main():
- global RELEASE_TAG, FEATURES_DIR, SHADER_TYPES
parser = argparse.ArgumentParser(description="Feature version audit for Skyrim Community Shaders.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| global RELEASE_TAG, FEATURES_DIR, SHADER_TYPES | |
| def main(): | |
| parser = argparse.ArgumentParser(description="Feature version audit for Skyrim Community Shaders.") | |
| # ... |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 569-569: global RELEASE_TAG is unused: name is never assigned in scope
(F824)
[error] 569-569: global FEATURES_DIR is unused: name is never assigned in scope
(F824)
[error] 569-569: global SHADER_TYPES is unused: name is never assigned in scope
(F824)
🤖 Prompt for AI Agents
In tools/feature_version_audit.py at line 569, remove the global declaration for
RELEASE_TAG, FEATURES_DIR, and SHADER_TYPES because these variables are not
reassigned within the function and do not require global scope declaration.
2 participants
Conducts feature audit checking including missing ini file warnings (warnings are optional for now).
Summary by CodeRabbit
New Features
Chores