Add mypy support and fixup project to give no errors #512

sveinse · 2024-07-10T11:01:15Z

With reference to #358 this PR is the starting point of adding typing with mypy support in canopen. It sets a recommended permissive starting point for mypy and it contains the absolute minimal code change required to pass all mypy errors.

It does contain behavioral changes that were required to pass the tests. The majority of these fixes are more graceful handling of errors that would occur anyways, so the behavioral change is what error they produce.

Add runtime test for self.network before using the network
PeriodicMessageTask.update() don't stop the task unless it's running
Variable.desc ensure that the object is int
Variable.read() fail with ValueError unless a valid fmt is used
Variable.write() ensure the description is a string
BaseNode.__init__() fail if no node_id is provided
ObjectDictionary.__getitem__() when splitting "." only return if the object is not an ODVariable
ODRecord.__eq__(), ODArray.__eq__() and ODVariable.__eq__() test type of other before comparing
ODVariable.encode_raw(), .decode_phys(), .encode_phys() add type tests of ensure the input is of correct type
PdoMap various methods: ensure necessary attributes are not None

* Permissive mypy configuration as starting point * Add minimal type annotations to get no mypy errors * Add runtime test for self.network before using the network * Network.add_node() doesn't accept LocalNode * PeriodicMessageTask.update() don't stop the task unless its running * Variable.desc ensure that the object is int * Variable.read() fail with ValueError unless a valid fmt is used * Variable.write() ensure the description is a string * BaseNode.__init__() fail if no node_id is provided * ObjectDictionary.__getitem__() when splitting "." only return if the object is not an ODVariable * ODRecord.__eq__(), ODArray.__eq__() and ODVariable.__eq__() test type of other before comparing * ODVariable.encode_raw(), .decode_phys(), .encode_phys() add type tests of ensure the input is of correct type * PdoMap various methods: ensure necessary attributes are set

codecov-commenter · 2024-07-10T11:03:25Z

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

Codecov Report

Attention: Patch coverage is 55.03356% with 67 lines in your changes missing coverage. Please review.

Project coverage is 67.72%. Comparing base (3aa509d) to head (3b66ae5).

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #512      +/-   ##
==========================================
- Coverage   68.78%   67.72%   -1.06%     
==========================================
  Files          26       26              
  Lines        3117     3198      +81     
  Branches      526      556      +30     
==========================================
+ Hits         2144     2166      +22     
- Misses        835      878      +43     
- Partials      138      154      +16

Files	Coverage Δ
canopen/node/local.py	`85.88% <100.00%> (+0.69%)`	⬆️
canopen/node/remote.py	`53.08% <100.00%> (ø)`
canopen/node/base.py	`83.33% <60.00%> (-16.67%)`	⬇️
canopen/sdo/base.py	`76.59% <57.14%> (-0.83%)`	⬇️
canopen/emcy.py	`68.42% <20.00%> (-3.41%)`	⬇️
canopen/variable.py	`53.33% <27.27%> (-2.79%)`	⬇️
canopen/network.py	`72.88% <50.00%> (-2.42%)`	⬇️
canopen/nmt.py	`86.71% <37.50%> (-6.52%)`	⬇️
canopen/objectdictionary/__init__.py	`79.57% <72.54%> (-3.11%)`	⬇️
canopen/pdo/base.py	`62.87% <33.33%> (-2.10%)`	⬇️

erlend-aasland · 2024-07-10T11:04:38Z

This is great! Can you separate out all the behavioural changes in separate PRs¹ accompanied with tests?

one PR per change in behaviour ↩

erlend-aasland · 2024-07-10T11:07:35Z

Just to be explicit: in #358 there is an agreement on not mixing type annotations with other types of changes, including bug fixes and behavioural changes.

sveinse · 2024-07-10T11:30:21Z

Yes, its possible to separate it out. While I see the intentions of separating out the type annotations from the behavioral changes, I'm not sure I like where this is going. Type annotating an existing project and behavioral changes goes iteratively hand in hand. Annotate something and an error pop up. Fix an error, and another annotation error appears. And so on. The sheer volume of PRs per behavioral change will be too great for me to handle. I won't be able to dedicate that much time to this. This PR is just a minor extract of what is to come ....at least if we're aiming for strict mypy. So perhaps we need to go back to #358 and really discuss what ambitions we have for type annotations to begin with.

erlend-aasland · 2024-07-10T11:54:07Z

Yes, it is time-consuming, and it requires multiple PRs. IMO it is definitely worth it in the long run.

acolomb · 2024-07-10T11:56:48Z

Yes, please let's first reach consensus on that discussion about the ambitions and the roadmap. Will chime in there soon, but I can't afford looking at it every day right now. Please have a little patience :-)

sveinse · 2024-07-10T12:07:34Z

How many pieces must this PR be split into?

erlend-aasland · 2024-07-10T12:26:55Z

How many pieces must this PR be split into?

From a quick glance at these changes, ISTM something like this is a sane approach¹:

Network instance is optional in many classes, which requires lot of checking #511
issue: discuss and agree on add_node / create_node changes
issue (similar to Network instance is optional in many classes, which requires lot of checking #511): what to do in BaseNode when node_id is None
issue (similar to the above): how to handle cob_id is None in PdoBase
PR: add WriteCallback / ReadCallback protocols (I believe these should not be public APIs, no?)
PR: harden OD encoding/decoding
PR: harden OD equality operator

I might have missed some; there's a lot of different types of changes here.

For each issue, time is best spent on reaching a consensus for a desired solution before proposing a PR. Anything else is often a waste of reviewer time. ↩

erlend-aasland · 2024-07-10T12:29:20Z

When the above preparations have landed, it will be time to introduce annotations, which should then be a much easier PR to review, as it would do only one thing: introduce annotations.

erlend-aasland · 2024-07-10T13:11:22Z

The sheer volume of PRs per behavioral change will be too great for me to handle. I won't be able to dedicate that much time to this.

That's fine; this is open source. Let me know if you want me to help out with any of the preliminary tasks.

sveinse · 2024-07-10T14:59:16Z

Please go ahead

acolomb

Gradually working through this. I had some of these fixes made locally as well, so trying to reconcile the bits and factor out into smaller PRs. I haven't checked all touched files yet, will do so as time permits.

This should be merged with master, especially after #525 is sanctioned and merged. It will reduce the diff a lot, making it easier to reason about.

acolomb · 2024-08-12T14:44:34Z

canopen/emcy.py

@@ -52,7 +53,7 @@ def reset(self):

    def wait(
        self, emcy_code: Optional[int] = None, timeout: float = 10
-    ) -> "EmcyError":
+    ) -> Optional[EmcyError]:


Cosmetics, let's keep these separate from the mypy error fixing. There are lots of things like this to do, but better to keep a PR focused and revisit such cleanups later. IMHO.

acolomb · 2024-08-12T14:44:52Z

canopen/emcy.py

+        if self.network is None:
+            raise RuntimeError("A Network is required")


Superseded by #525?

acolomb · 2024-08-12T14:45:24Z

canopen/network.py

+    # Type checkers don't like this conditional logic, so it is only run when
+    # not type checking
+    if not TYPE_CHECKING:
+        # Do not fail if python-can is not installed
+        can = None
+        CanError = Exception
+        class Listener:
+            """ Dummy listener """


Fixed in master, I believe?

acolomb · 2024-08-12T14:47:02Z

canopen/network.py

@@ -45,7 +51,7 @@ def __init__(self, bus: Optional[can.BusABC] = None):
        #: List of :class:`can.Listener` objects.
        #: Includes at least MessageListener.
        self.listeners = [MessageListener(self)]
-        self.notifier = None
+        self.notifier: Optional[can.Notifier] = None


Factored out to #534

acolomb · 2024-08-12T14:49:19Z

canopen/network.py

-            :class:`canopen.RemoteNode` or :class:`canopen.LocalNode` object.
+            :class:`canopen.RemoteNode` object.


I think what is wrong here is the return type annotation. It's perfectly alright to add an existing LocalNode to a network, isn't it?

acolomb · 2024-08-12T14:50:15Z

canopen/network.py

@@ -335,10 +345,12 @@ def update(self, data: bytes) -> None:
        old_data = self.msg.data
        self.msg.data = new_data
        if hasattr(self._task, "modify_data"):
+            assert self._task is not None  # This will never be None, but mypy needs this


Superseded by #534

acolomb · 2024-08-12T14:50:24Z

canopen/network.py

-            self._task.stop()
+            if self._task is not None:
+                self._task.stop()


Superseded by #534

sveinse mentioned this pull request Jul 10, 2024

Type annotations in canopen #358

Open

acolomb mentioned this pull request Aug 12, 2024

Fix some type checker errors in network module #534

Merged

acolomb requested changes Aug 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mypy support and fixup project to give no errors #512

Add mypy support and fixup project to give no errors #512

sveinse commented Jul 10, 2024

codecov-commenter commented Jul 10, 2024 •

edited

Loading

erlend-aasland commented Jul 10, 2024 •

edited

Loading

erlend-aasland commented Jul 10, 2024

sveinse commented Jul 10, 2024

erlend-aasland commented Jul 10, 2024

acolomb commented Jul 10, 2024

sveinse commented Jul 10, 2024

erlend-aasland commented Jul 10, 2024

erlend-aasland commented Jul 10, 2024

erlend-aasland commented Jul 10, 2024 •

edited

Loading

sveinse commented Jul 10, 2024

acolomb left a comment

acolomb Aug 12, 2024

acolomb Aug 12, 2024

acolomb Aug 12, 2024

acolomb Aug 12, 2024

acolomb Aug 12, 2024

acolomb Aug 12, 2024

acolomb Aug 12, 2024

		if self.network is None:
		raise RuntimeError("A Network is required")

		:class:`canopen.RemoteNode` or :class:`canopen.LocalNode` object.
		:class:`canopen.RemoteNode` object.

Add mypy support and fixup project to give no errors #512

Are you sure you want to change the base?

Add mypy support and fixup project to give no errors #512

Conversation

sveinse commented Jul 10, 2024

codecov-commenter commented Jul 10, 2024 • edited Loading

Codecov Report

erlend-aasland commented Jul 10, 2024 • edited Loading

Footnotes

erlend-aasland commented Jul 10, 2024

sveinse commented Jul 10, 2024

erlend-aasland commented Jul 10, 2024

acolomb commented Jul 10, 2024

sveinse commented Jul 10, 2024

erlend-aasland commented Jul 10, 2024

Footnotes

erlend-aasland commented Jul 10, 2024

erlend-aasland commented Jul 10, 2024 • edited Loading

sveinse commented Jul 10, 2024

acolomb left a comment

Choose a reason for hiding this comment

acolomb Aug 12, 2024

Choose a reason for hiding this comment

acolomb Aug 12, 2024

Choose a reason for hiding this comment

acolomb Aug 12, 2024

Choose a reason for hiding this comment

acolomb Aug 12, 2024

Choose a reason for hiding this comment

acolomb Aug 12, 2024

Choose a reason for hiding this comment

acolomb Aug 12, 2024

Choose a reason for hiding this comment

acolomb Aug 12, 2024

Choose a reason for hiding this comment

codecov-commenter commented Jul 10, 2024 •

edited

Loading

erlend-aasland commented Jul 10, 2024 •

edited

Loading

erlend-aasland commented Jul 10, 2024 •

edited

Loading