-
Notifications
You must be signed in to change notification settings - Fork 165
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
Feature/alarm #339
Feature/alarm #339
Conversation
7c8e732
to
6c4aa2f
Compare
…os into feature/alarm
I've noted some similarities with the other PRs. Could you please leave a note in them, which PR is based upon which other one? This would save me some time by not reviewing your changes twice (and commenting them :P ). |
d82bfbb
to
83a94ab
Compare
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.
...and when should this be merged? After or before #343 ?
The order does not matter, since they are independent features. And with any order there will be few merge conflicts 😃, but they are very easy to fix 😉. |
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.
Well, while the comment inside the unit test for the OswServiceTaskNotifier
may not be true anymore, I'm not a fan of implementing it myself... @cutieskye @akmal-threepointsix ?
A note regarding the "fixtures": A fixture is something which prepares / resets something for one specific unit-test. In the other tests those are used to start/prepare the emulator if e.g. interaction with the HAL is needed or used to reset/prepare the Preferences/NVS before every test run. Your unit-tests are therefore not really well placed inside the fixtures-folder. Furthermore, a fixture shall be independent of the test, so others could potentially reuse them as needed. |
I resolved all conflicts.
I removed outdated comment and added a new comment. Unfortunately, I don't have energy to write new unit tests 😢 |
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.
LGTM - We don't have a trigger module at the moment, but the idea is great!
Hmm, one check failed. But it looks like it is not our fault, it just could not download some package 🤔. Could someone restart all checks? |
Consider it done :) |
I'm sorry, but your branch is somewhat out-of-sync with the |
Erm sorry, I accidentally merged "develop" into this branch instead of rebasing. Is it ok? |
Nope, the "Merge Queue" is still rejecting this branch... If you can't solve it I would swing my admin-mode and hammer it in :) |
No description provided.