-
Notifications
You must be signed in to change notification settings - Fork 194
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
Add a dummy object to designate "uninitialized" networks (fixes #511) #525
base: master
Are you sure you want to change the base?
Conversation
This class can replace the dummy "None" value for attribute initializations, which can then be properly typed as Network to avoid static type checking errors. Switch to that new initialization in the NmtBase class as an example. This has the benefit of not needing `self.network is not None` checks at run-time wherever a method or attribute access is used, but still satisfies static type checking. When hitting such a code path at run-time, of course it will lead to an exception because the attributes required in the Network methods are not set. But that is a case of wrong API usage (accessing a network without associating it first), which a static checker cannot detect reliably. The dummy class could be modified to provide a better exception message if desired, but this is just a minimal proof of concept so far.
This works because `from foo import bar` requires full module initialization at import time, while simply importing the module name does not.
@erlend-aasland What do you think of this approach, before I start fleshing it out in more detail? |
I'm not totally convinced, but I like it better than sprinkling the code with conditionals. |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #525 +/- ##
=========================================
Coverage ? 71.20%
=========================================
Files ? 26
Lines ? 3129
Branches ? 527
=========================================
Hits ? 2228
Misses ? 770
Partials ? 131
|
Use a _DummyNetwork object instead.
Use a _DummyNetwork object instead. Fix up check for "network is None" by providing a convenience has_network() function in the base class.
Use a _DummyNetwork object instead.
Use a _DummyNetwork object instead.
Use a _DummyNetwork object instead.
Use a _DummyNetwork object instead.
canopen/network.py
Outdated
@@ -279,6 +279,17 @@ def __len__(self) -> int: | |||
return len(self.nodes) | |||
|
|||
|
|||
class _DummyNetwork(Network): |
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.
I'm not sure about the naming. I'd prefer if the naming more closely reflected it's purpose:
_NetworkPlaceholder
_UninitializedNetwork
_EmptyNetwork
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.
I like this!
(You need to amend the PR title to reflect the name changes.) |
This class can replace the dummy "None" value for attribute initializations, which can then be properly typed as Network to avoid static type checking errors.
Switch to that new initialization in the NmtBase class as an example.
This has the benefit of not needing
self.network is not None
checks at run-time wherever a method or attribute access is used, but still satisfies static type checking. When hitting such a code path at run-time, of course it will lead to an exception because the attributes required in the Network methods are not set. But that is a case of wrong API usage (accessing a network without associating it first), which a static checker cannot detect reliably. The dummy class could be modified to provide a better exception message if desired, but this is just a minimal proof of concept so far.This is an alternative approach to #513. If the concept looks acceptable, it will be extended to other places with the same type checker errors.