-
-
Notifications
You must be signed in to change notification settings - Fork 0
refactor(ia): fix type errors detected with ty #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors type-related interfaces by aligning method signatures and type hints between runtime code and stubs, and temporarily stubs out two OPC browse element property accessors that were causing type-checking issues. Class diagram for updated AbstractMutableJythonMap type signaturesclassDiagram
class PyObject
class MutableJythonMap
class AbstractMutableJythonMap {
+get(self, pyKey: PyObject, def_: PyObject) PyObject
+update(self, args: PyObject, kwargs: Union~str,unicode~) None
+__setitem__(self, pyKey: PyObject, pyValue: PyObject) None
+__delitem__(self, pyKey: PyObject) None
}
AbstractMutableJythonMap --|> MutableJythonMap
AbstractMutableJythonMap ..> PyObject
Class diagram for OPC BrowseElement property accessors after refactorclassDiagram
class BrowseElement {
+getItemType(self)
+getProperties(self) List~BrowseElement.PropertyElement~
+getProperty(self, id_: int) BrowseElement.PropertyElement
+getServer(self) Union~str,unicode~
}
class PropertyElement
BrowseElement o-- PropertyElement
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- In
opc.__init__.py,getPropertiesandgetPropertywere changed topass, which removes existing behavior and will break callers; instead, keep the implementations and adjust them for typing (e.g., wrapself._properties.values()inlist()and use appropriate typing/casting). - If the goal in
getPropertieswas to satisfy theList[BrowseElement.PropertyElement]return type, returninglist(self._properties.values())is preferable to removing the return entirely and will preserve existing runtime behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `opc.__init__.py`, `getProperties` and `getProperty` were changed to `pass`, which removes existing behavior and will break callers; instead, keep the implementations and adjust them for typing (e.g., wrap `self._properties.values()` in `list()` and use appropriate typing/casting).
- If the goal in `getProperties` was to satisfy the `List[BrowseElement.PropertyElement]` return type, returning `list(self._properties.values())` is preferable to removing the return entirely and will preserve existing runtime behavior.
## Individual Comments
### Comment 1
<location> `src/com/inductiveautomation/ignition/common/opc/__init__.py:185` </location>
<code_context>
def getProperties(self):
# type: () -> List[BrowseElement.PropertyElement]
- return self._properties.values()
+ pass
def getProperty(self, id_):
</code_context>
<issue_to_address>
**issue (bug_risk):** Replacing getProperties return with `pass` will break callers expecting a list of PropertyElement
This changes the method from returning a list-like collection of `BrowseElement.PropertyElement` to returning `None`, which will break existing callers (iteration, len, indexing, etc.). If the method is meant to be unimplemented, raise `NotImplementedError`; otherwise it should continue returning the underlying collection (or an appropriate wrapper).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def getProperties(self): | ||
| # type: () -> List[BrowseElement.PropertyElement] | ||
| return self._properties.values() | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Replacing getProperties return with pass will break callers expecting a list of PropertyElement
This changes the method from returning a list-like collection of BrowseElement.PropertyElement to returning None, which will break existing callers (iteration, len, indexing, etc.). If the method is meant to be unimplemented, raise NotImplementedError; otherwise it should continue returning the underlying collection (or an appropriate wrapper).
Summary by Sourcery
Align OPC and Jython script abstractions with updated typing expectations and stub definitions.
Enhancements: