search

MycroftAI / skill-alarm

Mycroft AI official Alarm Skill - Set single and recurring alarms, with a choice of alarm sounds
https://mycroft.ai/skills
Apache License 2.0
14 stars 32 forks source link

Event handling for active alarms #121

Closed chrisveilleux closed 3 years ago

chrisveilleux commented 3 years ago

Description

Implements generic "are there active alarms?" request and response events. Initial use case is the new Home Screen skill knowing when there are active alarms so the alarm indicator icon can be shown or hidden.

Type of PR

If your PR fits more than one category, there is a high chance you should submit more than one PR. Please consider this carefully before opening the PR.

Testing

Documentation

Docstrings updated.

NeonDaniel commented 3 years ago

Should a @skill_api_method be used here rather than new bus events? This would be consistent with the weather implementation here (https://github.com/MycroftAI/skill-homescreen/pull/2).

chrisveilleux commented 3 years ago

Should a @skill_api_method be used here rather than new bus events? This would be consistent with the weather implementation here (MycroftAI/skill-homescreen#2).

That PR will not be merged. This is a replacement to that PR. I've had some discussions with the internal team about the Skill API and I have some reservations about its use.

JarbasAl commented 3 years ago

I've had some discussions with the internal team about the Skill API and I have some reservations about its use.

does that mean https://github.com/MycroftAI/mycroft-core/pull/1822 is being deprecated?

NeonDaniel commented 3 years ago

Should a @skill_api_method be used here rather than new bus events? This would be consistent with the weather implementation here (MycroftAI/skill-homescreen#2).

That PR will not be merged. This is a replacement to that PR. I've had some discussions with the internal team about the Skill API and I have some reservations about its use.

Is there a roadmap or docs somewhere that discusses the future of this implementation or the desired alternative? Just trying to maintain compatibility with Mycroft where I can.

chrisveilleux commented 3 years ago

Slow down folks. One of the reasons the logic in this PR is implemented without the Skill API is to present a potential alternative solution. No decision has been made. We will be working on multiple aspects of skill interaction in our next sprint. Once we've gathered our thoughts on the matter internally, we will be communicating them publicly.

AIIX commented 3 years ago

the Skill API was clearly promoted to me as a solution to act as a pointer to other skill methods at the time of writing the homescreen skill instead of polluting the message bus with events that could eventually lead to other skills not be able to use same messages for where they could, so why shouldn't the Skill API be used for things like this ?

JarbasAl commented 3 years ago

Slow down folks. One of the reasons the logic in this PR is implemented without the Skill API is to present a potential alternative solution. No decision has been made. We will be working on multiple aspects of skill interaction in our next sprint. Once we've gathered our thoughts on the matter internally, we will be communicating them publicly.

You said categorically that the other PR would not be merged so it looked more like an announcement than a discussion, thanks for the clarification

the concern is that we are starting to add this in a bunch of skills and would hate to refactor again, so if a deprecation was planned we would look into alternative approaches like providing these decorators in an external package. A new major release is coming, if this is deprecated can we at least delay until the next major release so skill devs have time to adapt?

chrisveilleux commented 3 years ago

the Skill API was clearly promoted to me as a solution to act as a pointer to other skill methods at the time of writing the homescreen skill instead of polluting the message bus with events that could eventually lead to other skills not be able to use same messages for where they could, so why shouldn't the Skill API be used for things like this ?

The Skill API is not an alternative to "polluting the message bus". In fact, it emts message bus messages itself. It is, at its core, syntactic sugar around using message bus messages the same way I did in this PR to communicate data between skills.

chrisveilleux commented 3 years ago

You said categorically that the other PR would not be merged so it looked more like an announcement than a discussion, thanks for the clarification

It was an announcement that a PR would not be merged, not that the Skill API is going to be deprecated. Two very different things. Please do not conflate them into one. There are other reasons, besides Skill API usage, for this decision.

the concern is that we are starting to add this in a bunch of skills and would hate to refactor again, so if a deprecation was planned we would look into alternative approaches like providing these decorators in an external package. A new major release is coming, if this is deprecated can we at least delay until the next major release so skill devs have time to adapt?

Everyone needs to understand that Mycroft Core is not an architecturally stable platform yet. We release breaking changes every six months. Some breaking changes will deprecate existing methodologies. Some decisions that seemed like good ideas at one point in the project's life cycle will be reversed. If we are doing our jobs right, the amount of changes to existing architecture will decrease over time. We just aren't there yet.

The current plan is to complete the aforementioned skill interaction sprint before 21.08 so that it can be included in that release. This is being done because we anticipate breaking changes to be introduced in this sprint.

JarbasAl commented 3 years ago

Just asking that you don't remove it straight away, leave a proper deprecation warning log during the next 6 months, and after that actually remove it in the next major release

If a platform is not stable and simply removes core functionality out of nowhere developers will never feel comfortable developing for it and it will hurt adoption, with the deprecation warning and the 6 months time buffer devs can adapt to those breaking changes

EDIT: the implicit assumption here is that a deprecation warning would also point devs to the alternative implementation or mention that it will be removed and not replaced. ie, is the decision that this should not be done at all or that it should be done a different way

NeonDaniel commented 3 years ago

Slow down folks. One of the reasons the logic in this PR is implemented without the Skill API is to present a potential alternative solution. No decision has been made. We will be working on multiple aspects of skill interaction in our next sprint. Once we've gathered our thoughts on the matter internally, we will be communicating them publicly.

Thanks for the calrification, I'll look forward to that discussion of the Skill API when we get there. :)

krisgesling commented 3 years ago

Hi all, I wanted to follow up and reiterate that nothing is being deprecated. If we were to deprecate something like message replies, we'd first want to talk with the community about why the change was be proposed, what the alternative is, and how that alternative solves some problem or improves upon the current implementation. Then we'd put adequate deprecation warnings and compatibility layers in, all of this long before actually removing it.

The Skill interaction sprint that Chris mentioned is coming up. Leading into that piece of work Michael and Derick have been working on a discussion document outlining a proposal for the behaviour that we want to see from Mycroft Skills. We want to review that document with the community and make sure it reflects the behavioural direction we want to go. Once we have a good understanding of the behaviour we are aiming for, then together we can see what implementation would get us there.