fix: BaseTask.get_id potentially returning None#5047
Conversation
Agent-Logs-Url: https://github.com/ansys/pyfluent/sessions/7757a4b3-49ee-4d7e-9df4-6f8b2f8d9ac6 Co-authored-by: Gobot1234 <50501825+Gobot1234@users.noreply.github.com>
Agent-Logs-Url: https://github.com/ansys/pyfluent/sessions/7757a4b3-49ee-4d7e-9df4-6f8b2f8d9ac6 Co-authored-by: Gobot1234 <50501825+Gobot1234@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR prevents BaseTask.get_id() from implicitly returning None when the workflow state cannot be matched to the current task, avoiding downstream TypeErrors and making the failure explicit.
Changes:
- Add a terminal
RuntimeErrorinBaseTask.get_id()when no task ID can be resolved from the workflow state.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Up to standards ✅🟢 Issues
|
|
I don't think this really needs a test as it's covered by type checking |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # pylint: disable=missing-raises-doc | ||
| def get_id(self) -> str: | ||
| """Get the unique string identifier of this task, as it is in the application. |
There was a problem hiding this comment.
The # pylint: disable=missing-raises-doc directive here will apply beyond get_id() (until re-enabled), which can unintentionally suppress missing-raises-doc warnings for the rest of this large module. Prefer documenting the new RuntimeError in the docstring with a Raises section and removing the disable; if a disable is still needed, scope it to just this function and re-enable immediately after.
|
|
||
| def get_id(self) -> str: | ||
| # pylint: disable=missing-raises-doc | ||
| def get_id(self) -> str: |
There was a problem hiding this comment.
There is trailing whitespace after the function signature (def get_id(self) -> str:), which will typically trigger linting (e.g., W291). Please remove the extra spaces.
| def get_id(self) -> str: | |
| def get_id(self) -> str: |
| raise RuntimeError( | ||
| f"Task ID not found for task '{self.name()}'. " | ||
| "This may indicate the task was not properly initialized." | ||
| ) |
There was a problem hiding this comment.
This change introduces a new error path (raising RuntimeError when the task name is missing from workflow state). Please add/adjust a test to assert this behavior so regressions don’t reintroduce the previous implicit None return (and downstream TypeError).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/ansys/fluent/core/workflow.py:335
get_id()now raises, but the docstring still only documentsReturnsand the linter is suppressed with# pylint: disable=missing-raises-doc. Prefer documenting the new exception with aRaisessection (consistent with other methods in this file) and removing the pylint suppression so the docstring stays accurate going forward.
def get_id(self) -> str: # pylint: disable=missing-raises-doc
"""Get the unique string identifier of this task, as it is in the application.
Returns
-------
str
The string identifier.
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BaseTask.get_id()silently returnedNonewhen no matching task was found in the workflow state, causing an obscureTypeErrordownstream inget_idx()when attempting to subscript theNoneresult.Context
get_id()iterated over workflow state without a fallback, returningNoneon a miss. Callers likeget_idx()assumed astrreturn, producing:Change Summary
raise RuntimeError(...)at the end ofBaseTask.get_id()to surface the failure at the source with a clear, actionable message including the task name:Rationale
Failing fast with a descriptive error is preferable to propagating
Noneand producing a confusingTypeErrorseveral call frames away. Including the task name in the message aids debugging without requiring a stack trace inspection.Impact
Any code path that reaches
get_id()with a task not present in the workflow state will now raiseRuntimeErrorimmediately instead of silently returningNone. The return type contract (str) is now enforced at runtime.