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
Create mock wireless module test script #30
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.
Good work on your first PR!!! There's quite a few changes to make since the PR is pretty long since I got you after you were done. Usually, we like to keep the PR smaller so you don't need to make as much changes and it takes less time for people to review too! Let me know if you have any questions!
I would read https://github.com/monash-human-power/BOOST/blob/master/mqtt_max_speed.py to help you with bunching up the argparse
stuff
Also, feel free to add the whole of software team to be reviewers of this PR! |
When you submit a PR, your PR should also include a description of what the changes were, how to test it and expected results. |
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.
Will continue review on new files (GitHub got upset cos the code was all moved around)
'Either a single val that sets the average value for the sensor or and array of subvals' was changed to 'Either a single val that sets the average value for the sensor or an array of subvals'
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.
Looks good 👍
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.
Looking great! Sorry for the long review haha
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.
Some small stuff that I didn't pick up before oops.
c02 was changed to co2 which is what it is meant to be. GPS was also changed to lowercase to match the formating guide
b991f10
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.
Good work! Thanks for all the changes! Hopefully now you know a little bit more about PRs and Python! 👍
No description provided.