-
Notifications
You must be signed in to change notification settings - Fork 12
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
fix/abstract app init #130
Conversation
skill_id and bus are now handled in super() , the call to self._startup should not be done anymore
7974cff
to
7c93c02
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## dev #130 +/- ##
======================================
Coverage ? 58.96%
======================================
Files ? 33
Lines ? 3251
Branches ? 0
======================================
Hits ? 1917
Misses ? 1334
Partials ? 0 ☔ View full report in Codecov by Sentry. |
This PR doesn't fix the problem.. the bus is still reinitialized.. here is a stacktrace with this pr running (printed inside the bus client's ctor)
Somewhere inside the ctor inheritance chain the bus is being set to |
More debugging with log statemetns
So between this line and this line 🤯 |
expectation is PHAL -> OVOSAbstractApp -> OVOSSkill -> metaclass -> MycroftSkill , and args/kwargs get passed along in practice we get first the call to the metaclass with PHAL kwargs, and then the other init methods are called after that metaclass -> PHAL -> OVOSAbstractApp -> OVOSSkill -> MycroftSkill the metaclass is not working as desired, all OVOSAbstractApp will be broken i think, moving from |
skill_id and bus are now handled in super() , the call to self._startup should not be done anymore
makes OVOSSkill a subclass of BaseSkill and marks MycroftSkill for deprecation
the metaclass override of
__call__
is problematic for OVOSAbstractApps such as PHAL*, OCP, Persona and CommonIOT, it is also only there as a compat layer for classic mycroft skills, we avoid the issue by only applying that to the MycroftSkill class itself but not to OVOSSkill (since it's no longer a subclass of MycroftSkill)* PHAL error logs in comments below, fixed by not subclassing anymore from OVOSAbstractApp