narrow TypeError guard in newtons_second_law_of_motion#3
Conversation
…ix Python 3 multi-exception syntax in instagram_crawler
…xception — int() raises ValueError on non-numeric input, not a generic Exception
…emoved in Python 3, use parentheses instead
…oat*float can only raise TypeError for incompatible types; the bare Exception was masking real bugs from the caller
The bare 'except Exception' around mass * acceleration was catching RecursionError, MemoryError, KeyboardInterrupt, and any unrelated bug in surrounding code. The multiplication of two Python objects can only raise TypeError (wrong operand types) or OverflowError (very large floats), so narrowing to TypeError matches the documented error path and lets other exceptions propagate as designed. Also adds a doctest that exercises the error path with a None argument, so the new behavior is locked in by the test suite.
Reviewer's GuideThis PR narrows overly broad exception handling in multiple modules (notably in newtons_second_law_of_motion) to specific exception types, updates a DFS implementation to use vertex-keyed visitation with correct adjacency iteration, fixes a bug in an image negative demo, makes file handling in a Project Euler solution safer, and adds a doctest to lock in the TypeError behavior when invalid arguments are passed to the physics function. Flow diagram for newtons_second_law_of_motion error handlinggraph TD
A[newtons_second_law_of_motion called<br/>mass, acceleration] --> B[force = 0.0]
B --> C[Attempt force = mass * acceleration]
C --> D{TypeError raised?}
D -- Yes --> E[Return -0.0]
D -- No --> F[Return computed force]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR systematically modernizes Python 2-style code to Python 3 standards across 16 files, including exception handler syntax updates, refined exception type catching, graph traversal data structure refactoring, resource management improvements, and a display logic fix. ChangesPython 3 Compatibility and Code Modernization
Sequence Diagram(s)No sequence diagram is necessary for this PR, as the changes consist of syntax updates, exception handling refinement, data structure refactoring, and resource management improvements without introducing multi-component interactions or altered control flow that would benefit from visualization. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 1 issue, and left some high level feedback:
- In
graphs/depth_first_search_2.py,visited = {v: False for v in self.vertex}insidedfs()will use the adjacency-list entries themselves as keys (likely lists), which are unhashable; you probably want keys to be the vertex indices (e.g.,range(len(self.vertex))) to match howdfs_recursiveindexesself.vertex[start_vertex]andvisited[neighbor].
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `graphs/depth_first_search_2.py`, `visited = {v: False for v in self.vertex}` inside `dfs()` will use the adjacency-list entries themselves as keys (likely lists), which are unhashable; you probably want keys to be the vertex indices (e.g., `range(len(self.vertex))`) to match how `dfs_recursive` indexes `self.vertex[start_vertex]` and `visited[neighbor]`.
## Individual Comments
### Comment 1
<location path="physics/newtons_second_law_of_motion.py" line_range="73-74" />
<code_context>
100
>>> newtons_second_law_of_motion(2.0, 1)
2.0
+ >>> newtons_second_law_of_motion(None, 10)
+ -0.0
"""
force = 0.0
</code_context>
<issue_to_address>
**question (bug_risk):** Reconsider treating `None` as a valid input that silently returns `-0.0`.
This doctest effectively endorses `newtons_second_law_of_motion(None, 10)` returning `-0.0` as a normal result, conflating an invalid-parameter/sentinel condition with a legitimately possible physics value. That makes it difficult for callers to distinguish bad inputs from real results. Consider instead raising `TypeError`/`ValueError` for non-numeric `mass`/`acceleration`, or clearly documenting any sentinel behavior rather than folding it into the standard API.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| >>> newtons_second_law_of_motion(None, 10) | ||
| -0.0 |
There was a problem hiding this comment.
question (bug_risk): Reconsider treating None as a valid input that silently returns -0.0.
This doctest effectively endorses newtons_second_law_of_motion(None, 10) returning -0.0 as a normal result, conflating an invalid-parameter/sentinel condition with a legitimately possible physics value. That makes it difficult for callers to distinguish bad inputs from real results. Consider instead raising TypeError/ValueError for non-numeric mass/acceleration, or clearly documenting any sentinel behavior rather than folding it into the standard API.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
graphs/depth_first_search_2.py (1)
79-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the
visiteddocstring to match the new contract.The signature now takes a dict, but the parameter docs still say "A list to track visited vertices." That leaves the public API docs stale.
Suggested fix
- :param visited: A list to track visited vertices. + :param visited: A dict keyed by vertex ID to track visited vertices.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@graphs/depth_first_search_2.py` around lines 79 - 85, The docstring for dfs_recursive is out of date: the parameter visited is now a dict but the docs say "A list"; update the visited param description in the dfs_recursive docstring to describe the dict contract (e.g., mapping vertex -> bool or any marker of visited state), including expected key/value types and whether keys are added by the function, so the public API for dfs_recursive (method name dfs_recursive) accurately documents the visited: dict parameter.
🧹 Nitpick comments (1)
physics/newtons_second_law_of_motion.py (1)
65-75: ⚡ Quick winType hints don't reflect actual behavior.
The type signature declares
mass: float, but the new doctest and error handling demonstrate thatNoneis accepted (returning-0.0). Type checkers will flag calls likenewtons_second_law_of_motion(None, 10)as invalid, even though the runtime accepts it.Consider either:
- Tightening validation to reject non-float inputs and raise a descriptive error, or
- Updating type hints to
mass: float | None, acceleration: float | Noneif None is a valid inputAdditionally, using
-0.0as a sentinel value for invalid input is non-idiomatic and error-prone—callers can't reliably distinguish it from a legitimate zero-force result. Raising aTypeErrororValueErrorwith a clear message would be more explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@physics/newtons_second_law_of_motion.py` around lines 65 - 75, The function newtons_second_law_of_motion currently accepts None and returns -0.0; instead, tighten input validation by keeping the signature as mass: float, acceleration: float and adding explicit runtime checks at the top of newtons_second_law_of_motion to ensure mass and acceleration are numbers (int or float) and not None, raising a TypeError with a clear message like "mass must be a number" or "acceleration must be a number" when validation fails; remove the -0.0 sentinel behavior and update doctests to expect the TypeError for invalid inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@graphs/depth_first_search_2.py`:
- Around line 74-77: The issue is that add_edge() only ensures from_vertex
exists in self.vertex, so edges like add_edge(0, 1) leave sink vertices
unregistered and later DFS (in dfs_recursive or the traversal loop that builds
visited) raises KeyError; update add_edge(from_vertex, to_vertex) to also
register to_vertex in self.vertex (e.g., ensure self.vertex[to_vertex] exists
with an empty neighbor list or appropriate default) so all vertices referenced
by edges are materialized before dfs_recursive and the visited dict is
constructed.
- Line 74: Replace the dict comprehension that builds the visited map with
dict.fromkeys to satisfy Ruff C420: where visited is currently created as
"visited = {v: False for v in self.vertex}", change it to use
"dict.fromkeys(self.vertex, False)" (keep the same variable name `visited` and
same use of `self.vertex`) so the DFS function/method that references `visited`
continues to work identically while unblocking the linter.
---
Outside diff comments:
In `@graphs/depth_first_search_2.py`:
- Around line 79-85: The docstring for dfs_recursive is out of date: the
parameter visited is now a dict but the docs say "A list"; update the visited
param description in the dfs_recursive docstring to describe the dict contract
(e.g., mapping vertex -> bool or any marker of visited state), including
expected key/value types and whether keys are added by the function, so the
public API for dfs_recursive (method name dfs_recursive) accurately documents
the visited: dict parameter.
---
Nitpick comments:
In `@physics/newtons_second_law_of_motion.py`:
- Around line 65-75: The function newtons_second_law_of_motion currently accepts
None and returns -0.0; instead, tighten input validation by keeping the
signature as mass: float, acceleration: float and adding explicit runtime checks
at the top of newtons_second_law_of_motion to ensure mass and acceleration are
numbers (int or float) and not None, raising a TypeError with a clear message
like "mass must be a number" or "acceleration must be a number" when validation
fails; remove the -0.0 sentinel behavior and update doctests to expect the
TypeError for invalid inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6bc711be-3b2b-4df5-9dcf-441ea3275e2c
📒 Files selected for processing (15)
digital_image_processing/convert_to_negative.pydivide_and_conquer/convex_hull.pydynamic_programming/catalan_numbers.pygraphs/depth_first_search_2.pyhashes/enigma_machine.pyphysics/newtons_second_law_of_motion.pyproject_euler/problem_002/sol4.pyproject_euler/problem_003/sol1.pyproject_euler/problem_003/sol2.pyproject_euler/problem_003/sol3.pyproject_euler/problem_005/sol1.pyproject_euler/problem_007/sol2.pyproject_euler/problem_099/sol1.pyweb_programming/fetch_well_rx_price.pyweb_programming/instagram_crawler.py
| """ | ||
| # visited array for storing already visited nodes | ||
| visited = [False] * len(self.vertex) | ||
| visited = {v: False for v in self.vertex} |
There was a problem hiding this comment.
Use dict.fromkeys() to unblock Ruff.
Line 74 is the exact C420 failure from CI. dict.fromkeys(self.vertex, False) is equivalent here and satisfies Ruff.
Suggested fix
- visited = {v: False for v in self.vertex}
+ visited = dict.fromkeys(self.vertex, False)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| visited = {v: False for v in self.vertex} | |
| visited = dict.fromkeys(self.vertex, False) |
🧰 Tools
🪛 GitHub Actions: ruff / 0_ruff.txt
[error] 74-74: ruff check failed: C420 Unnecessary dict comprehension for iterable; use dict.fromkeys instead
🪛 GitHub Actions: ruff / ruff
[error] 74-74: ruff check failed (C420): Unnecessary dict comprehension for iterable; use dict.fromkeys instead
🪛 GitHub Check: ruff
[failure] 74-74: ruff (C420)
graphs/depth_first_search_2.py:74:19: C420 Unnecessary dict comprehension for iterable; use dict.fromkeys instead
help: Replace with dict.fromkeys(iterable))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@graphs/depth_first_search_2.py` at line 74, Replace the dict comprehension
that builds the visited map with dict.fromkeys to satisfy Ruff C420: where
visited is currently created as "visited = {v: False for v in self.vertex}",
change it to use "dict.fromkeys(self.vertex, False)" (keep the same variable
name `visited` and same use of `self.vertex`) so the DFS function/method that
references `visited` continues to work identically while unblocking the linter.
Sources: Linters/SAST tools, Pipeline failures
| visited = {v: False for v in self.vertex} | ||
| for vertex in self.vertex: | ||
| if not visited[vertex]: | ||
| self.dfs_recursive(vertex, visited) |
There was a problem hiding this comment.
Register sink vertices before traversing.
This refactor still assumes every neighbor is already a key in self.vertex, but add_edge() only inserts from_vertex. A valid graph like g.add_edge(0, 1); g.dfs() will hit visited[1] / self.vertex[1] and raise KeyError because vertex 1 was never materialized. The fix belongs in add_edge(), not in a defensive .get() here.
Suggested fix
def add_edge(self, from_vertex: int, to_vertex: int) -> None:
- if from_vertex in self.vertex:
- self.vertex[from_vertex].append(to_vertex)
- else:
- # else make a new vertex
- self.vertex[from_vertex] = [to_vertex]
+ self.vertex.setdefault(from_vertex, []).append(to_vertex)
+ self.vertex.setdefault(to_vertex, [])Also applies to: 104-107
🧰 Tools
🪛 GitHub Actions: ruff / 0_ruff.txt
[error] 74-74: ruff check failed: C420 Unnecessary dict comprehension for iterable; use dict.fromkeys instead
🪛 GitHub Actions: ruff / ruff
[error] 74-74: ruff check failed (C420): Unnecessary dict comprehension for iterable; use dict.fromkeys instead
🪛 GitHub Check: ruff
[failure] 74-74: ruff (C420)
graphs/depth_first_search_2.py:74:19: C420 Unnecessary dict comprehension for iterable; use dict.fromkeys instead
help: Replace with dict.fromkeys(iterable))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@graphs/depth_first_search_2.py` around lines 74 - 77, The issue is that
add_edge() only ensures from_vertex exists in self.vertex, so edges like
add_edge(0, 1) leave sink vertices unregistered and later DFS (in dfs_recursive
or the traversal loop that builds visited) raises KeyError; update
add_edge(from_vertex, to_vertex) to also register to_vertex in self.vertex
(e.g., ensure self.vertex[to_vertex] exists with an empty neighbor list or
appropriate default) so all vertices referenced by edges are materialized before
dfs_recursive and the visited dict is constructed.
1 participant
The bare
except Exceptionaroundmass * accelerationwas catching RecursionError, MemoryError, KeyboardInterrupt, and any unrelated bug in surrounding code. The multiplication of two Python objects can only raise TypeError (wrong operand types) or OverflowError (very large floats), so narrowing to TypeError matches the documented error path and lets other exceptions propagate as designed.Also adds a doctest that exercises the error path with a None argument, so the new behavior is locked in by the test suite.
Summary by Sourcery
Align several modules with modern Python error-handling and I/O practices while correcting logic errors in algorithms and examples.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
Bug Fixes
Refactor