Add a dummy object to designate "uninitialized" networks (fixes #511) #525
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.16% 71.37% +0.21%
==========================================
Files 26 26
Lines 3114 3130 +16
Branches 527 480 -47
==========================================
+ Hits 2216 2234 +18
+ Misses 766 765 -1
+ Partials 132 131 -1
|
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.
|
(You need to amend the PR title to reflect the name changes.) |
acolomb
changed the title
acolomb
changed the title
|
Gentle ping @sveinse, I'm still waiting on your opinion. If you're currently not able to invest much time, please say so. Two contributors with a preference to accept this PR is enough for me, just didn't want to pass you over. |
There was a problem hiding this comment.
@acolomb thank you for the ping. I think this solution is the best of the non-perfect solutions. The alternative would be to pepper with network: Optional[Network] = None and having to deal with the None to instruct the linter what it should be. Using this object shows intent and it will fail if it's tried used without initialization. PR Approved.
Sorry for not responding, I'm in a terrible busy phase @work so I have limited time. Thanks for the involvement thou.
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.