Skip to content
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

Annotate Network and check usage of it runtime #513

Conversation

sveinse
Copy link
Collaborator

@sveinse sveinse commented Jul 10, 2024

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
Copy link

⚠️ Please install the 'codecov app svg image' 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

Impacted file tree graph

@@            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
Copy link
Contributor

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
Copy link
Collaborator Author

sveinse commented Jul 10, 2024

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.

self.network = network
self.network: Network = network
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this deduced implicitly from the parameter type?

self.network = network
self.network: Optional[Network] = network
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, this seems duplicated.

@acolomb
Copy link
Collaborator

acolomb commented Aug 6, 2024

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?

@acolomb
Copy link
Collaborator

acolomb commented Oct 17, 2024

Superseded by #525.

@acolomb acolomb closed this Oct 17, 2024
@sveinse sveinse deleted the feature-network-annotation-and-handling branch October 17, 2024 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants