Skip to content
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

refactor SkillGUI class to import from ovos_utils #224

Merged
merged 3 commits into from Oct 25, 2022

Conversation

JarbasAl
Copy link
Member

there was a bunch of duplicated code and the API was starting to get out of sync with ovos_utils getting new features such as notifications support

there was a bunch of duplicated code and the API was starting to get out of sync with ovos_utils getting new features such as notifications support
@codecov
Copy link

codecov bot commented Oct 22, 2022

Codecov Report

Merging #224 (778afa2) into dev (6ceb058) will increase coverage by 2.12%.
The diff coverage is 36.75%.

@@            Coverage Diff             @@
##              dev     #224      +/-   ##
==========================================
+ Coverage   50.35%   52.47%   +2.12%     
==========================================
  Files         119      156      +37     
  Lines       10077     9154     -923     
==========================================
- Hits         5074     4804     -270     
+ Misses       5003     4350     -653     
Impacted Files Coverage Δ
mycroft/audio/__main__.py 0.00% <0.00%> (ø)
mycroft/client/enclosure/__main__.py 0.00% <0.00%> (ø)
mycroft/client/enclosure/mark1/arduino.py 0.00% <0.00%> (ø)
mycroft/client/enclosure/mark1/eyes.py 0.00% <0.00%> (ø)
mycroft/client/enclosure/mark1/mouth.py 0.00% <0.00%> (ø)
mycroft/client/speech/__main__.py 0.00% <0.00%> (ø)
mycroft/client/speech/hotword_factory.py 0.00% <0.00%> (-88.89%) ⬇️
mycroft/client/speech/service.py 0.00% <0.00%> (ø)
mycroft/client/speech/silence.py 0.00% <0.00%> (-42.86%) ⬇️
mycroft/client/text/__init__.py 0.00% <0.00%> (ø)
... and 149 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@AIIX
Copy link
Member

AIIX commented Oct 23, 2022

This will make GUI completely ovos-utils dependent and also means gui skills will also now indirectly become ovos utils dependent, i don't see how that is a good thing, and why it all needs to be moved outside core, if anything ovos-utils gui changes should be moved here instead for better sync than live outside core

@AIIX
Copy link
Member

AIIX commented Oct 23, 2022

request to avoid merging this till its better tested, seems it removes a lot of GUI specific code

@JarbasAl
Copy link
Member Author

This will make GUI completely ovos-utils dependent and also means gui skills will also now indirectly become ovos utils dependent, i don't see how that is a good thing, and why it all needs to be moved outside core, if anything ovos-utils gui changes should be moved here instead for better sync than live outside core

you added the new notification utils there because they are used in PHAL plugins etc, in ovos things other than skills can have a GUI interface.

there was no code removed here, all code you see deleted is the exact same function in ovos_utils,

ovos_utils is already a dependency of ovos-core. MycroftSkill class will remain working exactly the same, skills will not be tied to ovos_utils at all since they import from mycroft module. this doesnt even add a new dependency

also we cant really extend this class with new public methods just like skills, because then if a skill uses the new methods it wont run in mycroft-core. moving the changes from ovos-utils here is exactly the wrong thing to do since it breaks backwards compat and blocks plugins from having a GUI

@AIIX
Copy link
Member

AIIX commented Oct 23, 2022

This will make GUI completely ovos-utils dependent and also means gui skills will also now indirectly become ovos utils dependent, i don't see how that is a good thing, and why it all needs to be moved outside core, if anything ovos-utils gui changes should be moved here instead for better sync than live outside core

you added the new notification utils there because they are used in PHAL plugins etc, in ovos things other than skills can have a GUI interface.

there was no code removed here, all code you see deleted is the exact same function in ovos_utils,

ovos_utils is already a dependency of ovos-core. MycroftSkill class will remain working exactly the same, skills will not be tied to ovos_utils at all since they import from mycroft module. this doesnt even add a new dependency

also we cant really extend this class with new public methods just like skills, because then if a skill uses the new methods it wont run in mycroft-core. moving the changes from ovos-utils here is exactly the wrong thing to do since it breaks backwards compat and blocks plugins from having a GUI

This is still untested, and I would like to test this PR running on device with everything before I can say nothing breaks in GUI with this change

Copy link

@builderjer builderjer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been testing, and have no seen issues

AIIX
AIIX approved these changes Oct 25, 2022
@JarbasAl JarbasAl merged commit a7c578b into dev Oct 25, 2022
9 checks passed
@JarbasAl JarbasAl deleted the refactor/skillGUI_from_utils branch October 25, 2022 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor code refactor without functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants