Skip to content

Slcan review #611

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

Merged
merged 19 commits into from
Jun 6, 2019
Merged

Slcan review #611

merged 19 commits into from
Jun 6, 2019

Conversation

albertoscotta
Copy link

@albertoscotta albertoscotta commented Jun 2, 2019

General review of slcan.py

  • add read method to enhance abstraction
  • add flush method
  • add get_serial_number, to retrive the serial number of the CANbus-serial bridge (also add test case)
  • add get_version, to retrive the version of the CANbus-serial bridge (also add test case)
  • wrap up stuff related to bitrate setting in dedicated methods
  • make write method private/hidden

@codecov
Copy link

codecov bot commented Jun 2, 2019

Codecov Report

Merging #611 into develop will increase coverage by 0.15%.
The diff coverage is 76.34%.

@@            Coverage Diff             @@
##           develop    #611      +/-   ##
==========================================
+ Coverage    63.65%   63.8%   +0.15%     
==========================================
  Files           63      63              
  Lines         5544    5606      +62     
==========================================
+ Hits          3529    3577      +48     
- Misses        2015    2029      +14

@codecov
Copy link

codecov bot commented Jun 2, 2019

Codecov Report

Merging #611 into develop will increase coverage by 0.14%.
The diff coverage is 77.31%.

@@            Coverage Diff            @@
##           develop   #611      +/-   ##
=========================================
+ Coverage    63.85%    64%   +0.14%     
=========================================
  Files           63     63              
  Lines         5545   5607      +62     
=========================================
+ Hits          3541   3589      +48     
- Misses        2004   2018      +14

Copy link
Collaborator

@felixdivo felixdivo left a comment

Choose a reason for hiding this comment

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

At first: Thanks for helping!

I think we should document the newly added methods.

@felixdivo
Copy link
Collaborator

felixdivo commented Jun 5, 2019

While we're at it: shouldn't write() be hidden as well? It might need deprecation thought.

cc @hardbyte

Apart from that we can merge this.

@albertoscotta
Copy link
Author

While we're at it: shouldn't write() be hidden as well? It might need deprecation thought.

cc @hardbyte

Apart from that we can merge this.

You know, I was thinking the same yesterday when I was trying to amend for you comments, but I didn't want to go too far.
If 'read' is hidden, I reckon cleaner to have its counterpart 'write' hidden as well.
I will push a commit for it. If it is not all right I will just revert it.

@hardbyte
Copy link
Owner

hardbyte commented Jun 6, 2019

I think it is fine without a depreciation period - write should never have been exposed, and it isn't documented here

@felixdivo
Copy link
Collaborator

Nice! Thanks again @albertoscotta 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants