Refactor: Replace generic Exception with RuntimeError in Cell.add_agent#2950
Refactor: Replace generic Exception with RuntimeError in Cell.add_agent#2950falloficarus22 wants to merge 9 commits intomesa:mainfrom
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe change replaces a generic Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Performance benchmarks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mesa/discrete_space/cell.py (1)
112-115: Ruff TRY003: consider a domain-specific exception (or adjust rule/config)
Ruff flags Line 113 (TRY003). If this repo follows that rule strictly, define a small custom exception (e.g.,CellFullError(RuntimeError)) and raise that (optionally with a default message). Otherwise consider suppressing TRY003 locally for this line if the project doesn’t want bespoke exception types here.+class CellFullError(RuntimeError): + """Raised when adding an agent to a full cell is attempted.""" + def __init__(self, message: str = "Cell is full") -> None: + super().__init__(message) ... - if self.capacity and n >= self.capacity: - raise RuntimeError("Cell is full") + if self.capacity and n >= self.capacity: + raise CellFullError()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mesa/discrete_space/cell.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
mesa/discrete_space/cell.py
113-113: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
mesa/discrete_space/cell.py (1)
112-114: Good specificity upgrade; check downstream expectations for exception type
Switching toRuntimeErroris a nice improvement for callers/tests, but it can still be a behavioral break for any code asserting the exact exception type/message (or documentingException). Worth a quick scan/update of tests/docs that validate the failure mode ofCell.add_agent.
|
I am not sure using a |
mesa/discrete_space/cell.py
Outdated
| raise Exception( | ||
| "ERROR: Cell is full" | ||
| ) # FIXME we need MESA errors or a proper error | ||
| raise RuntimeError("Cell is full") |
There was a problem hiding this comment.
I think that the type of error isn't the primary concern, but the message. I think something like this would already be far more useful.
| raise RuntimeError("Cell is full") | |
| raise Exception( | |
| f"Cannot add agent {agent.unique_id} to cell at {self.coordinate}: " | |
| f"cell is full (capacity: {self.capacity}, current: {n})" | |
| ) # FIXME we might want dedicated MESA errors types |
@quaquel if we make a Mesa error type, could we use that to (automatically) include some information about:
- the model state (
model.timefor example by default) - where the error occurred (which agent, model, space, etc.)
There was a problem hiding this comment.
Not sure. I tend to keep errors simple with just a message. Including where is not needed because you already have the full stack trace, so you know where it occurred.
|
I have opened a discussion on mesa exceptions in the mesa 4.0 discussion. I am closing this pull request, but I have included a link to it for reference. Depending on how we decide to move forward, we might reopen this or at least draw heavily from it. |
3 participants
Description
This PR addresses a FIXME in mesa/discrete_space/cell.py by replacing a generic Exception with a more specific RuntimeError.
Changes
Motivation:
Using specific exception types allows for better error handling and testing. RuntimeError is semantically more appropriate here as it indicates a state error where the operation cannot be performed due to the current state of the cell (being full).
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.