-
Notifications
You must be signed in to change notification settings - Fork 202
Type annotate everywhere Network is used #575
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
Conversation
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 must have neglected to actually send this review last night...
One thing regarding the API, otherwise just styling hints. In the hope that we will soon in sync regarding style rules... 😉 My helpers are mostly just flake8
and sometimes black
for guidance. Just in case you want to let those run in the background of your IDE as well.
import struct | ||
import time | ||
from typing import Optional | ||
|
||
if TYPE_CHECKING: | ||
import canopen.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.
import canopen.network | |
import canopen.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.
The following code is not a class so it's strictly not needed to have two lines according to black. Is your preference to always have two spaces below imports?
(PS! Can I encourage you to write a coding guideline document? It certainly would lessen your need to review and comment on styling.)
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.
Is your preference to always have two spaces below imports?
Yes.
Sorry I won't have much time to invest for documentation in the coming weeks I'm afraid. Hope it gets better soon.
We could leave this unmerged and try to get #518 done first. Then this PR will be a perfect test case to see actual reductions in the mypy errors list. |
Would you be so kind to share your flake8 settings? I normally don't use flake8 (*), and if I use it out of box it only complains about "line too long" from network.py. It would help to have the same settings so I can get these formatting (*) I use pylance/pyright (as a part of VSCode), mypy and occasionally ruff and isort. Some projects use black rigorously |
Nothing special, just a more convenient line length. This is my
|
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
To finalize #511 this PR implements type annotations for
Network
everywhere in the codebase, making typing ofNework
feature complete.Network
is a central object and it makes development much easier when the (typing) references are resolvable everywhere.