-
Notifications
You must be signed in to change notification settings - Fork 15
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
Non-blocking functions #50
Conversation
68e54cd
to
727c57a
Compare
Can someone please review this pull request? @Letme |
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.
Nice contribution. Thanks for the effort. I just have few stylistic requests.
Sorry it took so long, I must have missed this somehow.
Ok, now CI is also running. Can I ask you to also add some tests for your changes, to make sure we do not break anything (and that we did not break anything now)? |
Regarding CI: I need to fix master, but I am waiting for Ceedling 1.0.0 release (this current version in master is fairly close to the 1.0.0, so it should be easy). That means that currently unit tests are not running as we are referencing non-existing pre-release https://github.com/ThrowTheSwitch/Ceedling/releases |
09b871d
to
fe7017a
Compare
Co-Authored-By: Crt Mori <crt.mori@gmail.com>
fe7017a
to
7316ce0
Compare
Thanks for taking the time to review! Appreciate the feedback! Your suggestions have been added. Sure, I'll write the tests. |
when this will get merged..? |
As soon as some tests are added to maintain functionality. I will do a local run with a latest pre-release to make sure that after fix of CI we have all green checks. Before for some reason CI was not run on external pull requests, but now we fixed this. The only problem is that Ceedling did not release 1.0.0 version yet. |
I've added the unit tests, please review the changes. I was able to locally run the tests successfully only when I changed |
I've fixed the |
Can you also run stylechecker, as it seems uncrusitfy complains about something? |
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.
Thanks for the complete contribution. I do not have the environment ready to do a complete temperature range check, I will rely on your and my manual tests. Once I fix ceedling with released gem also the CI will maintain the quality.
I think stylechecker complains about a line length, but that should be a quick run fix and then we merge.
I've run uncrustify, so the stylechecker is working successfully now! |
I'm sorry that stylechecker is complaining again. I used uncrustify latest version 0.79.0 and it locally ran successfully. The stylechecker workflow is using uncrustify version 0.75.1. This could be the reason of failure. |
I've updated the stylechecker to use the latest version of uncrustify. I think, it shouldn't complain now. Thanks! |
Great. Newer uncrustify is also one less worry for me to update. Thanks for the initiative. |
Glad to hear, happy to contribute. Thanks for your time and feedback! |
Provided functions enabling non-blocking operation (closes #49)