Annotate Network and check usage of it runtime

[sveinse]

With reference to #511 this PR properly annotates all uses of Network and adds runtime checks for the usage of Network instances. In very many classes self.network is Optional[Network] with default None and thus a runtime check is needed for it being set properly.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 35.29412% with 55 lines in your changes missing coverage. Please review.

Project coverage is 67.82%. Comparing base (3aa509d) to head (76e3729).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
- Coverage   68.78%   67.82%   -0.97%     
==========================================
  Files          26       26              
  Lines        3117     3173      +56     
  Branches      526      551      +25     
==========================================
+ Hits         2144     2152       +8     
- Misses        835      865      +30     
- Partials      138      156      +18     

Files	Coverage Δ
canopen/network.py	75.29% <100.00%> (ø)
canopen/node/base.py	81.81% <50.00%> (-18.19%)	⬇️
canopen/objectdictionary/eds.py	83.56% <60.00%> (-0.39%)	⬇️
canopen/sdo/base.py	75.78% <50.00%> (-1.63%)	⬇️
canopen/sdo/client.py	71.49% <0.00%> (-0.32%)	⬇️
canopen/sdo/server.py	86.82% <0.00%> (-1.37%)	⬇️
canopen/sync.py	59.09% <66.66%> (-4.07%)	⬇️
canopen/timestamp.py	88.88% <66.66%> (-11.12%)	⬇️
canopen/lss.py	41.17% <42.85%> (-0.26%)	⬇️
canopen/node/local.py	82.35% <37.50%> (-2.84%)	⬇️
... and 4 more

[erlend-aasland]

I'm not sure if sprinkling the code with a lot of ifs is the best way forward. Some of these changes are in functions/methods that can be assumed to be part of hot code, so it will potentially have a performance impact. Moreover, if it is decided to move forward with this PR, the exception messages should be rewritten to provide the user with a solution to the problem. Something like "This node is not associated with a network; did you forget to add it using add_node()?" or similar could be an improvement.

[sveinse]

I'm not sure if sprinkling the code with a lot of ifs is the best way forward. Some of these changes are in functions/methods that can be assumed to be part of hot code, so it will potentially have a performance impact.

Other alternatives is to mute the error with # type: ignore and let it fail in the next statement when self.network is None. The resulting error is not particularly helpful thou.

[acolomb]

Isn't this deduced implicitly from the parameter type?

[acolomb]

Same here, this seems duplicated.

[acolomb]

I agree that all these conditionals and RuntimeErrors are a bit distracting and verbose. Adding an assertion seems less intrusive and can be switched off to save runtime costs.

Other than that, the most basic question is, why should the network attribute be Optional? We could require it wherever possible, especially on classes that are not really a part of the public API. An alternative is to initialize to a dummy implementation instance that satisfies the required attributes. Haven't really investigated how involved that would be. Maybe a Protocol (PEP 544) could be useful for this purpose?