Feature/submodules #114
Open
Feature/submodules #114
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add support for git submodules in reference cache repositories: - Clone with --recurse-submodules - Pull and update submodules before branch setup 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Reviewer's GuideAdd support for git submodules in the cache repository workflow by enabling recursive cloning, replacing fetch with pull and submodule updates; update tests to validate these behaviors and include design specs. Sequence diagram for updated cache repository setup with submodule supportsequenceDiagram
participant Caller
participant "setup_cache_repo()"
participant "git"
Caller->>"setup_cache_repo()": Call with repo_spec
alt Repository does not exist
"setup_cache_repo()"->>"git": clone --recurse-submodules repo_url repo_dir
else Repository exists
"setup_cache_repo()"->>"git": pull (update repo)
"setup_cache_repo()"->>"git": submodule update --recursive --init
end
"setup_cache_repo()"-->>Caller: Return repo_dir
Class diagram for updated setup_cache_repo functionclassDiagram
class RepoSpec {
+repo_url: str
+repo_dir: pathlib.Path
}
class renv {
+setup_cache_repo(repo_spec: RepoSpec) pathlib.Path
}
renv --> RepoSpec: uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider refactoring the repeated subprocess.run git invocations into a shared helper utility to reduce duplication and ensure consistency.
- It may be helpful to add logging or error handling around the submodule update step to surface failures when initializing or updating submodules.
- Add a test case for nested submodules (multi-level) to ensure the --recursive flag correctly initializes deeper submodule hierarchies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider refactoring the repeated subprocess.run git invocations into a shared helper utility to reduce duplication and ensure consistency.
- It may be helpful to add logging or error handling around the submodule update step to surface failures when initializing or updating submodules.
- Add a test case for nested submodules (multi-level) to ensure the --recursive flag correctly initializes deeper submodule hierarchies.
## Individual Comments
### Comment 1
<location> `test/test_renv.py:196-198` </location>
<code_context>
- assert "git" in fetch_call[0][0]
- assert "fetch" in fetch_call[0][0]
- assert "--all" in fetch_call[0][0]
+ assert pull_call is not None
+ assert "git" in pull_call[0][0]
+ assert "pull" in pull_call[0][0]
+
+ # Check git submodule update was called
</code_context>
<issue_to_address>
**suggestion (testing):** Suggest adding a test for repositories without submodules.
Consider adding a test to verify behavior when no submodules are present, ensuring the update command handles this case gracefully.
Suggested implementation:
```python
# Check git submodule update was called
submodule_call = None
for call in mock_run.call_args_list:
if "submodule" in call[0][0]:
submodule_call = call
break
assert submodule_call is not None
assert "git" in submodule_call[0][0]
assert "submodule" in submodule_call[0][0]
assert "update" in submodule_call[0][0]
def test_git_pull_no_submodules(mocker):
"""
Test that git submodule update is not called when there are no submodules.
"""
# Setup: mock subprocess.run and simulate no submodules
mock_run = mocker.patch("subprocess.run")
# Simulate calls: only git pull, no submodule update
mock_run.call_args_list = [
(["git", "pull"],),
]
# Simulate function under test (replace with actual function call)
# e.g. update_repo(repo_path, has_submodules=False)
# For demonstration, we just check the calls
pull_call = None
for call in mock_run.call_args_list:
if "pull" in call[0][0]:
pull_call = call
break
assert pull_call is not None
assert "git" in pull_call[0][0]
assert "pull" in pull_call[0][0]
# Ensure submodule update was NOT called
submodule_call = None
for call in mock_run.call_args_list:
if "submodule" in call[0][0]:
submodule_call = call
break
assert submodule_call is None
```
- You may need to adjust the mock setup and function call in `test_git_pull_no_submodules` to match your actual implementation and how you determine the presence of submodules.
- If your codebase uses a helper or fixture for mocking subprocess calls, use that instead of direct patching.
- Ensure the new test is discovered by your test runner (pytest, unittest, etc).
</issue_to_address>
### Comment 2
<location> `test/test_renv.py:191-194` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 3
<location> `test/test_renv.py:192-194` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 4
<location> `test/test_renv.py:202-205` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 5
<location> `test/test_renv.py:203-205` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+196
to
+198
| assert pull_call is not None | ||
| assert "git" in pull_call[0][0] | ||
| assert "pull" in pull_call[0][0] |
Contributor
There was a problem hiding this comment.
suggestion (testing): Suggest adding a test for repositories without submodules.
Consider adding a test to verify behavior when no submodules are present, ensuring the update command handles this case gracefully.
Suggested implementation:
# Check git submodule update was called
submodule_call = None
for call in mock_run.call_args_list:
if "submodule" in call[0][0]:
submodule_call = call
break
assert submodule_call is not None
assert "git" in submodule_call[0][0]
assert "submodule" in submodule_call[0][0]
assert "update" in submodule_call[0][0]
def test_git_pull_no_submodules(mocker):
"""
Test that git submodule update is not called when there are no submodules.
"""
# Setup: mock subprocess.run and simulate no submodules
mock_run = mocker.patch("subprocess.run")
# Simulate calls: only git pull, no submodule update
mock_run.call_args_list = [
(["git", "pull"],),
]
# Simulate function under test (replace with actual function call)
# e.g. update_repo(repo_path, has_submodules=False)
# For demonstration, we just check the calls
pull_call = None
for call in mock_run.call_args_list:
if "pull" in call[0][0]:
pull_call = call
break
assert pull_call is not None
assert "git" in pull_call[0][0]
assert "pull" in pull_call[0][0]
# Ensure submodule update was NOT called
submodule_call = None
for call in mock_run.call_args_list:
if "submodule" in call[0][0]:
submodule_call = call
break
assert submodule_call is None- You may need to adjust the mock setup and function call in
test_git_pull_no_submodulesto match your actual implementation and how you determine the presence of submodules. - If your codebase uses a helper or fixture for mocking subprocess calls, use that instead of direct patching.
- Ensure the new test is discovered by your test runner (pytest, unittest, etc).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #114 +/- ##
==========================================
+ Coverage 66.54% 67.06% +0.51%
==========================================
Files 7 7
Lines 1405 1430 +25
==========================================
+ Hits 935 959 +24
- Misses 470 471 +1
🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add support for git submodules in the reference cache by cloning with --recurse-submodules and updating submodules on cache refresh, and update tests and documentation accordingly.
New Features:
Documentation:
Tests: