Add a dummy object to designate "uninitialized" networks (fixes #511) #525
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
I'm not sure about the naming. I'd prefer if the naming more closely reflected it's purpose:
_NetworkPlaceholder_UninitializedNetwork_EmptyNetwork
|
(You need to amend the PR title to reflect the name changes.) |
acolomb
changed the title
acolomb
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 Nonechecks 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.