Skip to content

Commit

Permalink
Deprecate Application-managed settings (#43)
Browse files Browse the repository at this point in the history
* Remove `settings` kwarg in OVOSAbstractApplication init and log error if passed
Update BaseSkill `settings` kwarg handling to use `_initial_settings`

* Remove deprecated cleanup of passed settings path

* Update settings init test to reflect expected behavior

* Log deprecation warning for settings and update test case

* Fix bug in backwards-compat. settings handling
  • Loading branch information
NeonDaniel authored Jan 19, 2023
1 parent b1468ae commit 4f1bc16
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 17 deletions.
8 changes: 6 additions & 2 deletions ovos_workshop/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from ovos_config.locations import get_xdg_config_save_path
from ovos_utils.messagebus import get_mycroft_bus
from ovos_utils.log import LOG

from ovos_workshop.resource_files import locate_lang_directories
from ovos_workshop.skills.ovos import OVOSSkill
Expand All @@ -10,8 +11,7 @@
class OVOSAbstractApplication(OVOSSkill):
def __init__(self, skill_id, bus=None, resources_dir=None,
lang=None, settings=None, gui=None, enable_settings_manager=False):
super().__init__(bus=bus, gui=gui, settings=settings,
resources_dir=resources_dir,
super().__init__(bus=bus, gui=gui, resources_dir=resources_dir,
enable_settings_manager=enable_settings_manager)
self.skill_id = skill_id
self._dedicated_bus = False
Expand All @@ -21,6 +21,10 @@ def __init__(self, skill_id, bus=None, resources_dir=None,
self._dedicated_bus = True
bus = get_mycroft_bus()
self._startup(bus, skill_id)
if settings:
LOG.warning("settings arg is deprecated and will be removed "
"in a future release")
self.settings.merge(settings)

@property
def _settings_path(self):
Expand Down
14 changes: 5 additions & 9 deletions ovos_workshop/skills/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ def __init__(self, name=None, bus=None, resources_dir=None,
#: Mycroft global configuration. (dict)
self.config_core = Configuration()

self._settings = settings
self._initial_settings = {}
self._settings = None
self._initial_settings = settings or dict()
self._settings_watchdog = None

#: Set to register a callback method that will be called every time
Expand Down Expand Up @@ -248,12 +248,7 @@ def _startup(self, bus, skill_id=""):

# initialize anything that depends on skill_id
self.log = LOG.create_logger(self.skill_id)
if self._settings and isinstance(self._settings, JsonStorage):
LOG.warning(f"Passing settings to __init__ is deprecated and"
f"will be removed in an upcoming release")
else:
LOG.debug(f"Initializing settings file")
self._init_settings()
self._init_settings()

# initialize anything that depends on the messagebus
self.bind(bus)
Expand Down Expand Up @@ -287,7 +282,8 @@ def _init_settings(self):
if self._initial_settings:
# TODO make a debug log in next version
LOG.warning("Copying default settings values defined in __init__ \n"
"Please move code from __init__() to initialize() if you did not expect to see this message")
"Please move code from __init__() to initialize() "
"if you did not expect to see this message")
for k, v in self._initial_settings.items():
if k not in self._settings:
self._settings[k] = v
Expand Down
10 changes: 4 additions & 6 deletions test/unittests/test_abstract_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,13 @@ def setUpClass(cls) -> None:
settings=cls.settings_obj, gui=cls.gui)
cls.app._startup(cls.bus)

@classmethod
def tearDownClass(cls) -> None:
remove(cls.test_path)

def test_settings_init(self):
self.assertEqual(self.app.settings, self.settings_obj)
self.assertNotEqual(self.app.settings, self.settings_obj)
self.assertFalse(self.app.settings['__mycroft_skill_firstrun'])
self.assertTrue(self.app.settings['test'])
self.assertFalse(self.app.settings['updated'])
self.settings_obj['updated'] = True
self.assertEqual(self.app.settings, self.settings_obj)
self.assertFalse(self.app.settings['updated'])

def test_settings_init_invalid_arg(self):
app = Application(skill_id="TestApplication",
Expand Down

0 comments on commit 4f1bc16

Please sign in to comment.