-
-
Notifications
You must be signed in to change notification settings - Fork 0
refactor(ia): fix type errors detected with ty #13
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 behavior to satisfy static type checker Class diagram for AbstractMutableJythonMap type alignmentclassDiagram
class PyObject
class MutableJythonMap
class AbstractMutableJythonMap {
+get(pyKey, def_) PyObject
+update(args, kwargs) void
+__setitem__(pyKey, pyValue) void
+__delitem__(pyKey) void
}
AbstractMutableJythonMap --|> MutableJythonMap
Class diagram for AbstractTagPath and OPC browse element type fixesclassDiagram
class Object
class TagPath
class AbstractTagPath {
+compareNullLow(c1, c2) int
+getChildPath(nextId) TagPath
}
class PropertyElement
class BrowseElement {
-_properties
+getItemType() void
+getProperties() List_BrowseElement_PropertyElement
+getProperty(id_) BrowseElement_PropertyElement
+getServer() String
}
AbstractTagPath --|> Object
AbstractTagPath --|> TagPath
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,BrowseElement.getPropertiesandgetPropertynow justpassinstead of returning_propertiesdata, which looks like a behavior regression—if this was done only to satisfy the type checker, consider returninglist(self._properties.values())/self._properties[id_]with appropriate typing instead of removing the implementation. - The removal of
AbstractTagPath.__init__changes the class’ initialization chain; if any subclasses relied onsuper(AbstractTagPath, self).__init__()being called, you may want to confirm that this is safe or move the super call into subclasses that need it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `opc.__init__.py`, `BrowseElement.getProperties` and `getProperty` now just `pass` instead of returning `_properties` data, which looks like a behavior regression—if this was done only to satisfy the type checker, consider returning `list(self._properties.values())` / `self._properties[id_]` with appropriate typing instead of removing the implementation.
- The removal of `AbstractTagPath.__init__` changes the class’ initialization chain; if any subclasses relied on `super(AbstractTagPath, self).__init__()` being called, you may want to confirm that this is safe or move the super call into subclasses that need it.
## 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):** getProperties now returns None instead of the expected collection of properties.
This used to return `self._properties.values()`, and now returns `None`, which will break any callers that iterate or inspect the properties and contradicts the declared return type (`List[BrowseElement.PropertyElement]`). If this is intended to be abstract, use `raise NotImplementedError` and update the type hint; otherwise, restore the previous return (or equivalent) to avoid a regression.
</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): getProperties now returns None instead of the expected collection of properties.
This used to return self._properties.values(), and now returns None, which will break any callers that iterate or inspect the properties and contradicts the declared return type (List[BrowseElement.PropertyElement]). If this is intended to be abstract, use raise NotImplementedError and update the type hint; otherwise, restore the previous return (or equivalent) to avoid a regression.
Summary by Sourcery
Align type signatures and abstractions across OPC, tag path, and script mapping modules.
Enhancements: