Feature/gh52 refactor and test review#53
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the sc.review flow into smaller, testable services and shared models, and adds unit tests to validate the new behavior.
Changes:
- Split responsibilities from
ReviewintoTicketService,GitHostService,RepoSourceimplementations, andPrompter. - Consolidated ticket/code-review/comment/repo-info dataclasses into
models.py(removingticket.pyandcode_review.py). - Added new unittest coverage for review execution and key helper behaviors; enhanced
run_tests.pyto accept a target test directory.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/review/test_ticket_service.py | Adds unit tests for TicketService resolve/update/branch matching/prompting. |
| tests/review/test_review.py | Adds unit tests for Review.run() flows and comment aggregation helpers. |
| tests/review/test_repo_info.py | Adds tests for RepoInfo.repo_slug parsing across URL formats. |
| tests/review/test_git_host_service.py | Adds tests for git host matching, instance creation, and CR URL generation. |
| src/sc/review/ticketing_instances/ticketing_instance.py | Updates Ticket import to centralized models. |
| src/sc/review/ticketing_instances/instances/redmine_instance.py | Updates Ticket import to centralized models. |
| src/sc/review/ticketing_instances/instances/jira_instance.py | Updates Ticket import to centralized models. |
| src/sc/review/ticket_service.py | Introduces TicketService for ticket resolution, update, and branch-to-ticket matching. |
| src/sc/review/ticket.py | Removes old Ticket dataclass (moved to models.py). |
| src/sc/review/review_config.py | Splits config into TicketHostConfig/GitHostConfig and corresponding Pydantic models. |
| src/sc/review/review.py | Refactors review workflow to use RepoSource, TicketService, GitHostService, and Prompter. |
| src/sc/review/repo_source/single_repo_source.py | Adds RepoSource for a single git repo. |
| src/sc/review/repo_source/repo_source.py | Adds RepoSource ABC and shared _get_repo_info helper. |
| src/sc/review/repo_source/manifest_repo_source.py | Adds RepoSource for repo manifests and project iteration. |
| src/sc/review/repo_source/init.py | Exposes repo source types from package init. |
| src/sc/review/prompter.py | Adds Prompter wrapper for interactive prompts. |
| src/sc/review/models.py | Adds consolidated dataclasses/enums for tickets, reviews, repo info, and comment rendering. |
| src/sc/review/main.py | Updates CLI entrypoints to use the refactored services/sources/config models. |
| src/sc/review/git_instances/instances/gitlab_instance.py | Updates imports and signature formatting for GitLab instance. |
| src/sc/review/git_instances/instances/github_instance.py | Updates imports and minor formatting for GitHub instance. |
| src/sc/review/git_instances/git_instance.py | Updates base interface types and return contract (`CodeReview |
| src/sc/review/git_instances/git_factory.py | Refactors factory and adds caching to instance creation. |
| src/sc/review/git_host_service.py | Adds GitHostService for git host matching and code review lookups. |
| src/sc/review/git_flow_branch_strategy.py | Adds strategy for computing target branch via git-flow detection. |
| src/sc/review/code_review.py | Removes old CodeReview/CRStatus definitions (moved to models.py). |
| run_tests.py | Allows selecting a test directory via argv (defaults to tests). |
Comments suppressed due to low confidence (1)
src/sc/review/git_instances/git_factory.py:32
GitFactory.createis missing@classmethodbut is called like a factory (GitFactory.create(...)). Without@classmethod, the first argument will bind to theclsparameter incorrectly, breaking provider creation at runtime. Add@classmethod(and ensure@cachewraps the correct callable) so calls bind correctly.
@cache
def create(cls, name: str, token: str, base_url: str | None) -> GitInstance:
try:
return cls._registry[name.lower()](token=token, base_url=base_url)
except KeyError:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
d64bef0 to
435b78c
Compare
TB-1993
left a comment
There was a problem hiding this comment.
I still have concerns about the structure of this. I feel that a lot of times the top level class is getting a data object, but nothing with it other than passing to another class to do stuff with. So the top level object isn't really the one making the decisions. But I think an SC wide refactor will correct a lot of this.
Refactor review for cleanliness and testability and then unittest it.