fix(oocana): return Optional[str] for node_id instead of magic strings#460
fix(oocana): return Optional[str] for node_id instead of magic strings#460leavesster wants to merge 1 commit intomainfrom
Conversation
Previously node_id returned "unknown" or "none" as special values, which is confusing and error-prone. Now it returns None when the node_id is unavailable, which is a clearer API contract.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Comment |
There was a problem hiding this comment.
Pull request overview
This PR changes the node_id property to return Optional[str] instead of magic strings ("unknown" and "none"), improving type safety and API clarity.
Changes:
- Modified
node_idproperty return type fromstrtoOptional[str] - Returns
Noneinstead of magic strings when node_id is unavailable - Added comprehensive docstring explaining when None is returned
- Updated internal usage at line 386 to handle Optional return type with fallback
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def node_id(self) -> Optional[str]: | ||
| """Get the node_id from the current execution stack. | ||
|
|
||
| Returns: | ||
| The node_id if available, or None when running in contexts | ||
| without a node_id (e.g., run_block). | ||
| """ | ||
| if len(self.__block_info.stacks) > 0: | ||
| return self.__block_info.stacks[-1].get("node_id", "unknown") | ||
| else: | ||
| return "none" | ||
| return self.__block_info.stacks[-1].get("node_id") | ||
| return None |
There was a problem hiding this comment.
The change from returning str to Optional[str] is a breaking API change. While the internal usage at line 386 has been properly updated to handle None, external callers of this property (such as executor/python_executor/block.py lines 134 and 146) are not updated in this PR and will break when node_id is None. These call sites use node_id directly in string formatting and file path construction, which will fail with None values.
Consider one of these approaches:
- Update all known call sites in the same PR to handle the Optional return type
- Add a deprecation period where the property returns "unknown" by default but logs a warning, giving consumers time to migrate
- Document this as a breaking change and coordinate updates across all dependent code
Summary
node_idproperty to returnOptional[str]instead of magic strings"unknown"and"none"Nonewhen node_id is not availableProblem
Original code returned magic strings:
This is problematic because:
Solution
Return
Nonewhen data is not available:Test Plan