-
-
Notifications
You must be signed in to change notification settings - Fork 4
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've had a read through and spotted a few places where things could be neatened up (although I appreciate this is work-in-progress code and you might already be aware of these).
Anyone interested in taking over this PR? I'm really struggling to find the motivation for this one. To do:
|
Co-Authored-By: Kier Davis <[email protected]>
I don't mind having a go at this but i'm not in a position to do so at the moment (my apartment doesn't have an internet connection yet) so it might be a week or two before I can work on it. I'll take over ownership of the issue once I'm available to work on it. |
This requires moving MockSerial out into its own file so it can be shared between test_motor_board.py and test_uno.py. Some rework is required since the two boards have different expected baud rates.
This makes the code a bit cleaner.
Previously it would reject "2020.1.0" since the month component is less than 6 despite the year component being greater than 2019.
This allows you to run 'make isort' to fix imports in all python source files.
This changes the order in which linting, testing and typechecking is done when the 'make' command is executed. Reasoning is that one usually wants to see and fix functional or type errors before dealing with linting errors.
@kierdavis How far off do you think this is? I'd like to see it merged by Friday in order to meet deadlines. |
I believe the implementation is complete, so I'll mark as ready for review. I'd still like to test this on hardware (with a multimeter or some LEDs plugged in to confirm the pins are controlled as expected), and maybe deal with the Code Climate warnings. I should be able to find time for that by Friday, but if not, feel free to go ahead and merge and I'll open a new PR if there's any further changes that need to be made. |
I'm happy for this to be merged. Looks good. |
Codeclimate is really broken, please ignore the warnings from it |
Code Climate has analyzed commit fdaaa76 and detected 2 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 83.8% (95% is the threshold). This pull request will bring the total coverage in the repository to 96.2% (-1.7% change). View more on Code Climate. |
As I'm the original author of this PR, Github will not let me approve. I do approve though, feel free to merge on admin override. |
Merged due to time constraints |
No description provided.