Add local exercise repo fetching for testing local changes #40
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for using local exercise repositories for testing, enabling developers to test exercise changes locally before pushing to remote. The implementation maintains backwards compatibility by defaulting to "remote" type when the type field is absent.
Changes:
- Extended
ExercisesSourcedataclass to support both "remote" and "local" repository types with appropriate fields for each - Added
from_rawclass method for flexible deserialization with backwards compatibility - Implemented local repository copying logic using
shutil.copytreefor isolation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| app/configs/gitmastery_config.py | Extended ExercisesSource to support local/remote types, added from_raw factory method for backwards-compatible deserialization |
| app/utils/gitmastery.py | Added local repo handling branch in enter method with path validation and directory copying logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if exercises_source.type == "local": | ||
| info(f"Using local exercises source at {exercises_source.path}") | ||
| # copy local repo into temp dir for isolation | ||
| if exercises_source.path is None: | ||
| raise ValueError("Path is required for using local exercises source") | ||
| src = Path(exercises_source.path).expanduser().resolve() | ||
| if not src.exists(): | ||
| raise FileNotFoundError(f"Local exercises source not found: {src}") | ||
| shutil.copytree(src, self.__temp_dir.name, dirs_exist_ok=True, symlinks=True, copy_function=shutil.copy2) | ||
| self.__repo = Repo(self.__temp_dir.name) |
There was a problem hiding this comment.
The checkout method calls sparse_checkout on the Git repo, which is specifically designed for sparse clones. When using a local exercises source, the copied repository may not be a sparse clone, which could cause the sparse_checkout command to fail or behave unexpectedly. Consider checking if the repository is a sparse clone before calling sparse_checkout, or handling local sources differently in the checkout, has_file, and fetch_file_contents methods.
There was a problem hiding this comment.
It works fine during testing.
app/utils/gitmastery.py
Outdated
| info(f"Using local exercises source at {exercises_source.path}") | ||
| # copy local repo into temp dir for isolation | ||
| if exercises_source.path is None: | ||
| raise ValueError("Path is required for using local exercises source") | ||
| src = Path(exercises_source.path).expanduser().resolve() |
There was a problem hiding this comment.
The path validation check occurs after already logging the path. If path is None, the log message will display "None" before raising the ValueError. Consider moving the validation check before the info log to provide clearer error handling flow.
| info(f"Using local exercises source at {exercises_source.path}") | |
| # copy local repo into temp dir for isolation | |
| if exercises_source.path is None: | |
| raise ValueError("Path is required for using local exercises source") | |
| src = Path(exercises_source.path).expanduser().resolve() | |
| # copy local repo into temp dir for isolation | |
| if exercises_source.path is None: | |
| raise ValueError("Path is required for using local exercises source") | |
| src = Path(exercises_source.path).expanduser().resolve() | |
| info(f"Using local exercises source at {src}") |
app/utils/gitmastery.py
Outdated
| src = Path(exercises_source.path).expanduser().resolve() | ||
| if not src.exists(): | ||
| raise FileNotFoundError(f"Local exercises source not found: {src}") | ||
| shutil.copytree(src, self.__temp_dir.name, dirs_exist_ok=True, symlinks=True, copy_function=shutil.copy2) |
There was a problem hiding this comment.
Using symlinks=True in shutil.copytree could lead to issues if the local exercises source contains symlinks pointing to locations outside the repository. Consider using symlinks=False or adding validation to ensure symlinks only point to locations within the source directory.
| typ = raw.get("type") | ||
| # explicit local | ||
| if typ == "local": | ||
| return cls(type="local", path=raw.get("path")) | ||
| # legacy/detected remote | ||
| return cls( |
There was a problem hiding this comment.
When the type field is not present in the dict (backwards compatibility case), the code defaults to treating it as "remote". However, the from_raw method gets the type using raw.get("type"), which returns None when the key is not present. The conditional check 'if typ == "local"' will skip this None case, correctly falling through to the legacy remote case. Consider adding a comment to clarify this intended backwards compatibility behavior.
app/utils/gitmastery.py
Outdated
| src = Path(exercises_source.path).expanduser().resolve() | ||
| if not src.exists(): | ||
| raise FileNotFoundError(f"Local exercises source not found: {src}") | ||
| shutil.copytree(src, self.__temp_dir.name, dirs_exist_ok=True, symlinks=True, copy_function=shutil.copy2) |
There was a problem hiding this comment.
The code copies the local path and then initializes it as a Repo, but it doesn't validate that the local path is actually a git repository. If the path exists but is not a git repository, Repo(self.__temp_dir.name) will fail with an unclear error. Consider validating that the local path is a git repository before copying, or provide a clearer error message if it's not.
| shutil.copytree(src, self.__temp_dir.name, dirs_exist_ok=True, symlinks=True, copy_function=shutil.copy2) | |
| if not (src / ".git").is_dir(): | |
| raise ValueError(f"Local exercises source is not a git repository: {src}") | |
| shutil.copytree( | |
| src, | |
| self.__temp_dir.name, | |
| dirs_exist_ok=True, | |
| symlinks=True, | |
| copy_function=shutil.copy2, | |
| ) |
There was a problem hiding this comment.
The error is InvalidGitRepositoryError, which is mostly clear.
|
|
||
| def to_url(self) -> str: | ||
| if self.type != "remote": | ||
| raise ValueError("to_url only valid for remote ExercisesSource") |
There was a problem hiding this comment.
The to_url method references self.username and self.repository without checking if they are None. For remote type ExercisesSource instances, these fields could be None if not properly validated during construction. Consider adding validation to ensure username and repository are not None when type is "remote".
| raise ValueError("to_url only valid for remote ExercisesSource") | |
| raise ValueError("to_url only valid for remote ExercisesSource") | |
| if self.username is None or self.repository is None: | |
| raise ValueError("Remote ExercisesSource requires 'username' and 'repository' to be set") |
- Typecheck `path` beofre use - Remove symlink copying - Clarify legacy fallthrough
|
@VikramGoyal23 Is this ready for review? Would also be good to resolve copilot comments if they have been addressed as well to reduce cognitive load on reviewer |
2 participants
Allows developers to set the exercise source as a local path, enabling fully local exercise testing. Backwards compatible and assumes lack of
typefield asremote.