--only-changed-files fixes #59
Conversation
…d improving error messages
There was a problem hiding this comment.
Pull Request Overview
This PR refactors Git integration to improve handling of changed files filtering by centralizing Git information in a GitInfo object and fixing issues with Git reference handling.
- Centralizes Git-related information (repository, commit SHA, refs) in a unified
GitInfoobject - Replaces separate
base_refandtarget_refparameters with structured Git information - Improves error handling and validation for Git operations
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/codeql_wrapper/infrastructure/git_utils.py | Enhanced GitInfo class with new fields and refactored get_git_info method to accept optional parameters |
| src/codeql_wrapper/domain/use_cases/codeql_analysis_use_case.py | Updated changed files detection to use Git info from request object |
| src/codeql_wrapper/domain/entities/codeql_analysis.py | Replaced separate ref parameters with GitInfo object in CodeQLAnalysisRequest |
| src/codeql_wrapper/cli.py | Updated CLI to use GitInfo object and removed target_ref option |
| pyproject.toml | Version bump from 0.2.0-alpha.2 to 0.2.0-alpha.5 |
| repository_path: Path, | ||
| base_ref: str = "HEAD~1", | ||
| target_ref: str = "HEAD", | ||
| base_ref: str, |
There was a problem hiding this comment.
Removing default values from function parameters without providing alternatives could break existing code. Consider providing default values or documenting the breaking change clearly.
| base_ref: str, | |
| base_ref: Optional[str] = None, |
Copilot uses AI. Check for mistakes.
| base_ref: str = "HEAD~1", | ||
| target_ref: str = "HEAD", | ||
| base_ref: str, | ||
| target_ref: str, |
There was a problem hiding this comment.
Removing default values from function parameters without providing alternatives could break existing code. Consider providing default values or documenting the breaking change clearly.
| target_ref: str, | |
| target_ref: Optional[str] = None, |
Copilot uses AI. Check for mistakes.
| except Exception: | ||
| logger = get_logger(__name__) | ||
|
|
||
| if base_ref is None: |
There was a problem hiding this comment.
The parameters are typed as str but the code checks for None. This creates a type mismatch. Either change the type annotation to Optional[str] or remove the None check.
Copilot uses AI. Check for mistakes.
| if base_ref is None: | ||
| base_ref = GitUtils._get_current_ref(repository_path) | ||
|
|
||
| if target_ref is None: |
There was a problem hiding this comment.
The parameters are typed as str but the code checks for None. This creates a type mismatch. Either change the type annotation to Optional[str] or remove the None check.
Copilot uses AI. Check for mistakes.
src/codeql_wrapper/cli.py
Outdated
| if only_changed_files and not git_info.is_git_repository: | ||
| click.echo( | ||
| click.style("ERROR:", fg="red", bold=True) | ||
| + " --only-changed-files requires a Git repository" | ||
| ) | ||
| sys.exit(1) | ||
|
|
||
| if git_info.base_ref is None: | ||
| click.echo( | ||
| click.style("ERROR:", fg="red", bold=True) | ||
| + "No base reference provided. Please use --base-ref option to specify it." | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
This validation occurs unconditionally but should only apply when only_changed_files is True. Moving this check inside the only_changed_files condition would be more appropriate.
Copilot uses AI. Check for mistakes.
No description provided.