Conversation
Contributor
Reviewer's GuideRefactors resolve_submodule_path to use angle‐bracket syntax, normalize numeric attributes, support slicing and function calls, and adds a new submodule_path_to_name helper, updating tests throughout to cover the new behavior. Class diagram for updated submodule path handling functionsclassDiagram
class hooks {
+merge_paths(paths: str) str
+resolve_submodule_path(root: nn_Module, path: str)
+submodule_path_to_name(path: str) str
}
class nn_Module {
}
nn_Module : <<external>>
hooks ..> nn_Module : uses as root argument
Class diagram for new submodule_path_to_name helperclassDiagram
class hooks {
+submodule_path_to_name(path: str): str
}
Flow diagram for submodule path resolution logicflowchart TD
A["Input path string"] --> B["Normalize numeric attributes with angle brackets"]
B --> C["Remove redundant dots and strip leading/trailing dots"]
C --> D{"Does path contain '<...>'?"}
D -- Yes --> E["Split at '<', recursively resolve left and right"]
D -- No --> F["Evaluate path using eval (attribute/index/function call)"]
E --> G["Return resolved submodule"]
F --> G
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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/tdhook/hooks.py:122` </location>
<code_context>
try:
- if key.startswith("["):
- return eval(f"root{key}", {"__builtins__": {}}, safe_dict)
+ if path.startswith(("[", ".")):
+ return eval(f"root{path}", {"__builtins__": {}}, safe_dict)
else:
</code_context>
<issue_to_address>
**🚨 issue (security):** The eval usage for dynamic path resolution could pose security risks if path input is not strictly controlled.
Even with __builtins__ removed, eval remains unsafe for user-controlled input. Use a safer parsing method or stricter input validation if paths can be influenced externally.
</issue_to_address>
### Comment 2
<location> `tests/test_hooks.py:476-477` </location>
<code_context>
assert resolve_submodule_path(root, "items[-1]") == "third"
assert resolve_submodule_path(root, "items[-2]") == "second"
+ # Test slice indexing
+ assert resolve_submodule_path(root, "items[1:3]") == ["second", "third"]
+
def test_dict_indexing(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for out-of-range and negative slice indices.
Please add tests for out-of-range slices, negative indices, and empty slices to ensure all edge cases are covered.
```suggestion
# Test slice indexing
assert resolve_submodule_path(root, "items[1:3]") == ["second", "third"]
# Test out-of-range slice indices
assert resolve_submodule_path(root, "items[2:10]") == ["third"]
assert resolve_submodule_path(root, "items[10:20]") == []
# Test negative slice indices
assert resolve_submodule_path(root, "items[-3:-1]") == ["first", "second"]
assert resolve_submodule_path(root, "items[-2:]") == ["second", "third"]
assert resolve_submodule_path(root, "items[:-1]") == ["first", "second"]
# Test empty slices
assert resolve_submodule_path(root, "items[1:1]") == []
assert resolve_submodule_path(root, "items[2:2]") == []
assert resolve_submodule_path(root, "items[3:3]") == []
```
</issue_to_address>
### Comment 3
<location> `tests/test_hooks.py:492-493` </location>
<code_context>
- def test_custom_attributes_with_colons(self):
- """Test custom attributes using colon syntax."""
+ def test_custom_attributes_with_angles(self):
+ """Test custom attributes using angles syntax."""
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for malformed angle bracket syntax and nested angle brackets.
Please include tests for malformed angle bracket syntax, such as missing brackets and nested brackets, to verify that ValueError is raised as expected.
```suggestion
def test_custom_attributes_with_angles(self):
"""Test custom attributes using angles syntax."""
def test_malformed_angle_bracket_syntax(self):
"""Test malformed angle bracket syntax raises ValueError."""
# Missing closing bracket
with pytest.raises(ValueError):
resolve_submodule_path(root, "items<custom")
# Missing opening bracket
with pytest.raises(ValueError):
resolve_submodule_path(root, "items custom>")
# Only one angle bracket
with pytest.raises(ValueError):
resolve_submodule_path(root, "items<custom>")
# Empty angle brackets
with pytest.raises(ValueError):
resolve_submodule_path(root, "items<>")
def test_nested_angle_brackets(self):
"""Test nested angle brackets raises ValueError."""
# Nested angle brackets
with pytest.raises(ValueError):
resolve_submodule_path(root, "items<outer<inner>>")
with pytest.raises(ValueError):
resolve_submodule_path(root, "items<<nested>>")
```
</issue_to_address>
### Comment 4
<location> `tests/test_hooks.py:556-564` </location>
<code_context>
assert resolve_submodule_path(root, "layers[0].items[1]") == "b"
assert resolve_submodule_path(root, "name") == "root"
+ def test_function_call(self):
+ """Test function call."""
+
+ class DummyRoot:
+ def __init__(self):
+ self.fn = lambda x: x + 1
+
+ root = DummyRoot()
+ assert resolve_submodule_path(root, "fn(0)") == 1
+
def test_invalid_paths_raise_value_error(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for invalid function calls and multiple arguments.
Please add tests for cases such as calling non-callable attributes and passing incorrect argument types or counts, as well as for multiple arguments, to verify robust error handling.
```suggestion
def test_function_call(self):
"""Test function call and error handling for function calls."""
class DummyRoot:
def __init__(self):
self.fn = lambda x: x + 1
self.fn_multi = lambda x, y: x * y
self.value = 42
root = DummyRoot()
# Valid single argument call
assert resolve_submodule_path(root, "fn(0)") == 1
# Valid multiple argument call
assert resolve_submodule_path(root, "fn_multi(2, 3)") == 6
# Non-callable attribute
with pytest.raises(ValueError):
resolve_submodule_path(root, "value(1)")
# Incorrect argument type
with pytest.raises(Exception):
resolve_submodule_path(root, "fn('not_an_int')")
# Incorrect argument count
with pytest.raises(Exception):
resolve_submodule_path(root, "fn_multi(1)")
```
</issue_to_address>
### Comment 5
<location> `tests/test_hooks.py:566-568` </location>
<code_context>
+ root = DummyRoot()
+ assert resolve_submodule_path(root, "fn(0)") == 1
+
def test_invalid_paths_raise_value_error(self):
"""Test that invalid paths raise ValueError."""
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding more invalid path cases for new syntax.
Please include tests for invalid angle bracket and function call syntax to verify ValueError is raised.
```suggestion
def test_invalid_paths_raise_value_error(self):
"""Test that invalid paths raise ValueError."""
class DummyRoot:
def __init__(self):
self.fn = lambda x: x + 1
self.items = ["first", "second", "third"]
root = DummyRoot()
import pytest
# Invalid angle bracket syntax
with pytest.raises(ValueError):
resolve_submodule_path(root, "items<0>")
with pytest.raises(ValueError):
resolve_submodule_path(root, "fn<1>")
# Invalid function call syntax
with pytest.raises(ValueError):
resolve_submodule_path(root, "fn()") # missing argument
with pytest.raises(ValueError):
resolve_submodule_path(root, "fn(,)") # invalid argument
with pytest.raises(ValueError):
resolve_submodule_path(root, "fn(1,2)") # too many arguments
# Existing invalid path cases can remain here
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
==========================================
+ Coverage 96.28% 96.29% +0.01%
==========================================
Files 30 30
Lines 1909 1918 +9
==========================================
+ Hits 1838 1847 +9
Misses 71 71 ☔ View full report in Codecov by Sentry. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What does this PR do?
Key insights about the PR.
Linked Issues
Summary by Sourcery
Improve submodule path handling by migrating from colon-based to angle-bracket custom attribute syntax, adding support for slicing and function calls in resolve_submodule_path, and introducing a submodule_path_to_name helper function.
New Features:
Enhancements:
Tests: