Merged
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Reviewer's GuideAdds a new parameterized decorator example and associated documentation, introduces a Pydantic default-values section, improves formatting and outputs in several notebooks, and updates documentation metadata (site title and environment requirements). Sequence diagram for the new parameterized decorator call flowsequenceDiagram
actor User
participant silly as silly_decorated
participant wrapper as wrapper
participant silly_orig as silly_original
User->>silly: call silly(0.5)
activate silly
silly->>wrapper: delegate call(0.5)
deactivate silly
activate wrapper
wrapper->>wrapper: check min_value <= x <= max_value
alt x within bounds
wrapper->>silly_orig: call x**2 - 2.0
activate silly_orig
silly_orig-->>wrapper: result
deactivate silly_orig
wrapper-->>User: result
else x out of bounds
wrapper-->>User: raise ValueError
end
deactivate wrapper
Class diagram for new decorator and Pydantic Agent modelclassDiagram
class BaseModel
class Field
class Agent {
+str name
+bool interactive = False
+list~str~ tools = Field(default_factory=list)
}
BaseModel <|-- Agent
class DecoratorsModule {
+check_bounds(min_value, max_value) check_bounds_wrapper
+check_bounds_wrapper(_func) wrapper
+wrapper(x) float
+silly(x) float
}
class CheckBoundsClosure {
+float min_value
+float max_value
+check_bounds_wrapper(_func) wrapper
}
class WrapperFunction {
+float min_value
+float max_value
+wrapper(x) float
}
DecoratorsModule o-- CheckBoundsClosure
CheckBoundsClosure o-- WrapperFunction
DecoratorsModule ..> Agent : independent example
DecoratorsModule ..> BaseModel : no direct dependency
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}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
decorator_arguments.py, thewrappercurrently only accepts a single positional argumentx; if you intend this decorator to be reusable, consider changing the signature towrapper(*args, **kwargs)and extracting the value to check, so it works with functions that take multiple or keyword arguments while preserving call semantics. - The error handling in
decorator_arguments.pyusese.message, which is not a standard attribute onValueErrorin modern Python; usestr(e)(orrepr(e)) in the f-string instead to ensure the exception message is printed correctly. - The new README entry under
source-code/decorators/README.mdrefers to acheck_rangedecorator, but the implementation is namedcheck_bounds; aligning the names will avoid confusion for readers looking for the example.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `decorator_arguments.py`, the `wrapper` currently only accepts a single positional argument `x`; if you intend this decorator to be reusable, consider changing the signature to `wrapper(*args, **kwargs)` and extracting the value to check, so it works with functions that take multiple or keyword arguments while preserving call semantics.
- The error handling in `decorator_arguments.py` uses `e.message`, which is not a standard attribute on `ValueError` in modern Python; use `str(e)` (or `repr(e)`) in the f-string instead to ensure the exception message is printed correctly.
- The new README entry under `source-code/decorators/README.md` refers to a `check_range` decorator, but the implementation is named `check_bounds`; aligning the names will avoid confusion for readers looking for the example.
## Individual Comments
### Comment 1
<location path="source-code/decorators/decorator_arguments.py" line_range="20-28" />
<code_context>
+ '''
+ def check_bounds_wrapper(_func):
+ @functools.wraps(_func)
+ def wrapper(x):
+ if min_value > x or x > max_value:
+ raise ValueError(f'argument {x} not in [{min_value}, {max_value}]')
+ return _func(x)
</code_context>
<issue_to_address>
**suggestion:** Decorator only supports a single positional argument; consider generalizing to *args/**kwargs or making this constraint explicit.
Currently `check_bounds` will throw a generic `TypeError` if used on a function with more than one argument, because `wrapper` only accepts `x`. To make this safer, you could define `wrapper(*args, **kwargs)`, forward all arguments to `_func`, and explicitly check the relevant argument (e.g., `args[0]`). If the decorator is intentionally limited to single-argument functions, consider validating `_func`’s signature up front so incorrect usage fails with a clear error message.
```suggestion
'''
def check_bounds_wrapper(_func):
@functools.wraps(_func)
def wrapper(*args, **kwargs):
if not args:
raise TypeError(
"check_bounds-decorated functions must have at least one "
"positional argument to check"
)
x = args[0]
if min_value > x or x > max_value:
raise ValueError(f'argument {x} not in [{min_value}, {max_value}]')
return _func(*args, **kwargs)
return wrapper
return check_bounds_wrapper
```
</issue_to_address>
### Comment 2
<location path="source-code/decorators/decorator_arguments.py" line_range="51-53" />
<code_context>
+if __name__ == '__main__':
+ print(silly(0.5))
+ try:
+ print(silly(2.5))
+ except ValueError as e:
+ print(f'Excepton raised as expected: {e.message}')
</code_context>
<issue_to_address>
**issue (bug_risk):** Catching ValueError and using `e.message` will fail; use `str(e)` (and fix the typo) instead.
In Python 3, `ValueError` instances don’t have a `.message` attribute, so `e.message` will raise an `AttributeError` instead of showing the original error. Use `str(e)` or interpolate `e` directly, e.g.:
```python
except ValueError as e:
print(f"Exception raised as expected: {e}")
```
Also, fix the typo: `Excepton` → `Exception`.
</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
+20
to
+28
| ''' | ||
| def check_bounds_wrapper(_func): | ||
| @functools.wraps(_func) | ||
| def wrapper(x): | ||
| if min_value > x or x > max_value: | ||
| raise ValueError(f'argument {x} not in [{min_value}, {max_value}]') | ||
| return _func(x) | ||
| return wrapper | ||
| return check_bounds_wrapper |
There was a problem hiding this comment.
suggestion: Decorator only supports a single positional argument; consider generalizing to *args/**kwargs or making this constraint explicit.
Currently check_bounds will throw a generic TypeError if used on a function with more than one argument, because wrapper only accepts x. To make this safer, you could define wrapper(*args, **kwargs), forward all arguments to _func, and explicitly check the relevant argument (e.g., args[0]). If the decorator is intentionally limited to single-argument functions, consider validating _func’s signature up front so incorrect usage fails with a clear error message.
Suggested change
| ''' | |
| def check_bounds_wrapper(_func): | |
| @functools.wraps(_func) | |
| def wrapper(x): | |
| if min_value > x or x > max_value: | |
| raise ValueError(f'argument {x} not in [{min_value}, {max_value}]') | |
| return _func(x) | |
| return wrapper | |
| return check_bounds_wrapper | |
| ''' | |
| def check_bounds_wrapper(_func): | |
| @functools.wraps(_func) | |
| def wrapper(*args, **kwargs): | |
| if not args: | |
| raise TypeError( | |
| "check_bounds-decorated functions must have at least one " | |
| "positional argument to check" | |
| ) | |
| x = args[0] | |
| if min_value > x or x > max_value: | |
| raise ValueError(f'argument {x} not in [{min_value}, {max_value}]') | |
| return _func(*args, **kwargs) | |
| return wrapper | |
| return check_bounds_wrapper |
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.
Summary by Sourcery
Document and demonstrate additional concepts in notebooks and docs, including default values in Pydantic models and decorator arguments with bounds checking, alongside minor formatting and configuration improvements.
New Features:
Enhancements: