Implement include_remote_repo option#246
Conversation
|
Hi @jiaxinnns, thank you for your contribution! 🎉 This PR comes from your fork Before you request for a review, please ensure that you have tested your changes locally! Important The previously recommended way of using Please read the following instructions for the latest instructions. PrerequisitesEnsure that you have the Testing stepsIf you already have a local Git-Mastery root to test, you can skip the following step. Create a Git-Mastery root locally: gitmastery setupNavigate into the Git-Mastery root (defaults to cd gitmastery-exercises/Edit the {
# other fields...
"exercises_source": {
"username": "jiaxinnns",
"repository": "exercises",
"branch": "feature/include-remote-repo"
}
}Then, you can use the gitmastery download <your new change>
gitmastery verifyChecklist
Important To any reviewers of this pull request, please use the same instructions above to test the changes. |
There was a problem hiding this comment.
Pull request overview
This PR implements an include_remote_repo option for the test utilities, allowing tests to optionally create and manage a remote repository alongside the main repository. This feature is intended to support testing scenarios that require interaction between local and remote repositories.
Changes:
- Added
include_remote_repoparameter toGitAutograderTestandGitAutograderTestLoader.start()method - Modified
__enter__to optionally create and return a remote RepoSmith instance - Added type overloads to
start()method to provide correct type hints based on whether remote repo is included
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.__rs.add_helper(GitMasteryHelper) | ||
|
|
||
| return self, self.rs | ||
| return self, self.rs, self.rs_remote |
There was a problem hiding this comment.
The __enter__ method now always returns three values (test, rs, rs_remote) regardless of whether include_remote_repo is True or False. When include_remote_repo is False, rs_remote will be None. This design decision means all callers must handle three return values, even when they don't need the remote repo. While this simplifies the implementation, it breaks backward compatibility with existing code that unpacks only two values (e.g., with test as (ctx, rs):). Consider whether this breaking change is acceptable, or if a different design would be better.
| return self, self.rs, self.rs_remote | |
| if self.include_remote_repo: | |
| return self, self.rs, self.rs_remote | |
| return self, self.rs |
There was a problem hiding this comment.
This makes type arguments complicated and results in failure in mypy test cases, so we only extract out the relevant variables at start() based on include_remote_repo
jovnc
left a comment
There was a problem hiding this comment.
Overall LGTM! Thanks for the work, as it seems that the typing is tricky here since we have 2 possible yield permutations now.
We will need to think about how we can extend this in the future, if we want to support more remotes, but let's not over-engineer for now and this is good enough for what we need.
| mock_answers: Optional[Dict[str, str]] = None, | ||
| ) -> Iterator[Tuple[GitAutograderTest, RepoSmith]]: | ||
| include_remote_repo: bool = False, | ||
| ) -> Iterator[Any]: |
There was a problem hiding this comment.
Generally, I would avoid using Any as type, but it seems to make the most sense here.
I tried returning Tuple[GitAutograderTest, RepoSmith, RepoSmith] | Tuple[GitAutograderTest, RepoSmith] but it seems that Python's type system can't narrow a union return type based on instance attributes at call time.
Having the @overload here seems to be the best solution for type safety.
There was a problem hiding this comment.
Agree, without @overload, users lose the precision of return type when include_remote_repo is set to True/False.
|
@desmondwong1215 @SAN-MUYUN @VikramGoyal23 Do feel free to also leave a review as well, and see if this change fits your exercise. We can merge it in once we are ready, so that it can unblock you guys |
SAN-MUYUN
left a comment
There was a problem hiding this comment.
Thanks so much for the contribution! Apart from the 1 tiny coding style issue, everything else LGTM. This should be sufficient for the current exercises. 👍
| remote_repo_path = remote_temp_path / repo_name | ||
| os.makedirs(remote_repo_path, exist_ok=True) | ||
| self.__rs_remote_context = create_repo_smith( | ||
| False, existing_path=remote_repo_path.absolute().as_posix() |
There was a problem hiding this comment.
Just a small remark, for consistency, existing_path... should be on a new line.
There was a problem hiding this comment.
The code be auto-formatted by ruff
| mock_answers: Optional[Dict[str, str]] = None, | ||
| ) -> Iterator[Tuple[GitAutograderTest, RepoSmith]]: | ||
| include_remote_repo: bool = False, | ||
| ) -> Iterator[Any]: |
There was a problem hiding this comment.
Agree, without @overload, users lose the precision of return type when include_remote_repo is set to True/False.
Fixes #247
Add support for
include_remote_repooption to create two repo-smith objects.Potential future improvement: allow creation of more than two repo-smith objects
Example usage in
test_verify.py: