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

Add typing annotations for functions in can.bus #652

Merged
merged 7 commits into from
Jul 29, 2019

Conversation

karlding
Copy link
Collaborator

This adds typing annotations for use via mypy for all functions under
can.bus.

This works towards PEP 561 compatibility.

@karlding
Copy link
Collaborator Author

I wasn't sure how to handle annotating stop_all_periodic_tasks, since we wrap the original stop class method and then add support for the remove_tasks argument, so each implementation doesn't need to provide it. Thus, wrapped_stop_method is called first in the call stack before the original stop method.

Adding annotations to stop_all_periodic_tasks would result in that function being checked by mypy, and I wasn't sure how to resolve the following:

can/bus.py:291: error: Unexpected keyword argument "remove_task" for "stop" of "CyclicTask"

As such, I've skipped it for now, but any ideas or insights would be appreciated!

This adds typing annotations for use via mypy for all functions under
can.bus.

This works towards PEP 561 compatibility.
@codecov
Copy link

codecov bot commented Jul 23, 2019

Codecov Report

Merging #652 into develop will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           develop    #652      +/-   ##
==========================================
+ Coverage    67.67%   67.7%   +0.03%     
==========================================
  Files           68      68              
  Lines         6098    6104       +6     
==========================================
+ Hits          4127    4133       +6     
  Misses        1971    1971

@codecov
Copy link

codecov bot commented Jul 23, 2019

Codecov Report

Merging #652 into develop will increase coverage by 0.05%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop     #652      +/-   ##
===========================================
+ Coverage    67.67%   67.73%   +0.05%     
===========================================
  Files           68       69       +1     
  Lines         6098     6109      +11     
===========================================
+ Hits          4127     4138      +11     
  Misses        1971     1971

can/bus.py Outdated Show resolved Hide resolved
@hardbyte
Copy link
Owner

@karlding given the change in #637 I think we can just remove the remove_task=False from stop_all_periodic_tasks altogether?

Add a Type Alias for CAN Filters used by can.bus
can/bus.py Show resolved Hide resolved
can/bus.py Show resolved Hide resolved
With the introduction of the sphinx-autodoc-typehints extension, we
don't need to duplicate typing information in the docstring as well as
the function signature.
@felixdivo
Copy link
Collaborator

@karlding given the change in #637 I think we can just remove the remove_task=False from stop_all_periodic_tasks altogether?

But it you then called task.stop() directly, it wouldn't be removed from the bus, right?

@hardbyte hardbyte merged commit 0019438 into hardbyte:develop Jul 29, 2019
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.

3 participants