nimmo/forgejo-auto-pr-review
Watch
1
0
Fork
You've already forked forgejo-auto-pr-review
 0
Code Issues 5 Pull requests Projects Releases Packages 1 Activity Actions

fix: accept scalar exclude paths #24

Merged
nimmo merged 1 commit from issue-23-exclude-paths-string into main 2026-07-23 19:07:31 +01:00
Conversation 2 Commits 1 Files changed 3 +23 -5
nimmo commented 2026-07-23 18:44:07 +01:00
Owner
Copy link

Closes #23

Accept review.exclude_paths as a YAML list, a single path string, or comma-separated path strings. Invalid types now receive an error that names the accepted forms.

Validation: python3 -m unittest discover -s tests (42 tests); nix flake check.

Closes #23 Accept `review.exclude_paths` as a YAML list, a single path string, or comma-separated path strings. Invalid types now receive an error that names the accepted forms. Validation: `python3 -m unittest discover -s tests` (42 tests); `nix flake check`.
fix: accept scalar exclude paths
All checks were successful
Build and publish container image / prepare-tags (push) Successful in 1s
Details
Build and publish container image / build-and-push (push) Successful in 15s
Details
AI pull-request review / review (pull_request) Successful in 6m44s
Details
3d0ada3863
nimmo commented 2026-07-23 19:04:05 +01:00
Author
Owner
Copy link

Automated code review

Reviewed commit: 3d0ada386381cf84a0e76a79df1815242af866b6

Verdict: Ready to merge

No blocking or important findings were retained, and deterministic checks passed. Suggestions remain advisory.

Overall assessment

The pull request introduces normalize_exclude_paths to support scalar strings, comma-separated strings, and YAML lists for review.exclude_paths, replacing inline validation in main(). Documentation is updated to reflect the new input formats. Unit tests cover valid inputs, whitespace trimming, empty string filtering, and invalid type rejection.

Blocking findings

None.

Important findings

None.

Suggestions

  • Consider accepting tuples or generic sequences for exclude_paths (reviewer.py:496; medium confidence)
    if isinstance(value, list) and all(isinstance(pattern, str) for pattern in value):
    Impact: Strictly checking for list may reject valid sequence types like tuples if YAML parsing or internal config structures change.
    Suggested fix: Consider using isinstance(value, (list, tuple)) or checking against collections.abc.Sequence while explicitly excluding strings to improve type flexibility without compromising safety.

Tests and validation

  • unit-tests: success

Questions

  • Does the configuration parser guarantee that YAML lists are always deserialized as Python list objects, or could they occasionally be tuples/other sequences depending on the YAML library version?

Review limitations

  • Review is based solely on the provided diff and test output; runtime behavior with actual Forgejo event payloads and Ollama integration was not executed.
  • Type hints and imports were not fully visible in the diff, so external dependency compatibility was assumed.

Diff coverage

  • README.md: reviewed — included in a context-limited batch
  • reviewer.py: reviewed — included in a context-limited batch
  • tests/test_reviewer.py: reviewed — included in a context-limited batch
Review metadata
  • Reviewer: forgejo-ai-review v1
  • Model: qwen3.6:35b-a3b-q8_0
  • Guidance loaded: .forgejo/ai-review.yml, README.md
  • Commits considered: 1
  • Previous automated review: none found
  • Findings: 1
<!-- forgejo-ai-review:v1 --> ## Automated code review Reviewed commit: `3d0ada386381cf84a0e76a79df1815242af866b6` ## Verdict: Ready to merge No blocking or important findings were retained, and deterministic checks passed. Suggestions remain advisory. ### Overall assessment The pull request introduces `normalize_exclude_paths` to support scalar strings, comma-separated strings, and YAML lists for `review.exclude_paths`, replacing inline validation in `main()`. Documentation is updated to reflect the new input formats. Unit tests cover valid inputs, whitespace trimming, empty string filtering, and invalid type rejection. ### Blocking findings None. ### Important findings None. ### Suggestions - **Consider accepting tuples or generic sequences for exclude_paths** (reviewer.py:496; medium confidence) if isinstance(value, list) and all(isinstance(pattern, str) for pattern in value): Impact: Strictly checking for `list` may reject valid sequence types like tuples if YAML parsing or internal config structures change. Suggested fix: Consider using `isinstance(value, (list, tuple))` or checking against `collections.abc.Sequence` while explicitly excluding strings to improve type flexibility without compromising safety. ### Tests and validation - unit-tests: **success** ### Questions - Does the configuration parser guarantee that YAML lists are always deserialized as Python `list` objects, or could they occasionally be tuples/other sequences depending on the YAML library version? ### Review limitations - Review is based solely on the provided diff and test output; runtime behavior with actual Forgejo event payloads and Ollama integration was not executed. - Type hints and imports were not fully visible in the diff, so external dependency compatibility was assumed. ### Diff coverage - `README.md`: **reviewed** — included in a context-limited batch - `reviewer.py`: **reviewed** — included in a context-limited batch - `tests/test_reviewer.py`: **reviewed** — included in a context-limited batch <details><summary>Review metadata</summary> - Reviewer: forgejo-ai-review v1 - Model: qwen3.6:35b-a3b-q8_0 - Guidance loaded: .forgejo/ai-review.yml, README.md - Commits considered: 1 - Previous automated review: none found - Findings: 1 </details>
nimmo commented 2026-07-23 19:05:21 +01:00
Author
Owner
Copy link

Final review assessment: no blocking or important findings remain. I am not extending exclude_paths to tuples or arbitrary sequences: the supported YAML subset and JSON configuration formats both produce Python lists, which is the deliberate public configuration contract. The PR is ready to merge.

Final review assessment: no blocking or important findings remain. I am not extending `exclude_paths` to tuples or arbitrary sequences: the supported YAML subset and JSON configuration formats both produce Python lists, which is the deliberate public configuration contract. The PR is ready to merge.
nimmo merged commit 084b254c5a into main 2026-07-23 19:07:31 +01:00
nimmo deleted branch issue-23-exclude-paths-string 2026-07-23 19:07:32 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
nimmo/forgejo-auto-pr-review!24
No description provided.