refactor/standardize_usage_of_skill_id (#28)

This commit changes every usage of skill name to the deterministic skill_id

There are several references in the code base to MycroftSkill self.name, this is not deterministic, by default it takes the name of the class but can be defined by skill developers and change in each skill update.

self.name/self.skill_id are used in the __init__ method, but skill_id is not ensured to be defined at this step, potentially introducing issues depending on when certain methods are called, this is particularly troublesome when skills use the __init__ method since behavior may change depending on when super() is called

Author: JarbasAl

mycroft/skills/event_scheduler.py

@@ -165,8 +165,7 @@ def schedule_event(self, event, sched_time, repeat=None,
 
             # Don't schedule if the event is repeating and already scheduled
             if repeat and event in self.events:
-                LOG.debug('Repeating event {} is already scheduled, discarding'
-                          .format(event))
+                LOG.debug(f'Repeating event {event} is already scheduled, discarding')
             else:
                 # add received event and time
                 event_list.append((sched_time, repeat, data, context))
@@ -241,7 +240,7 @@ def get_event_handler(self, message):
         with self.event_lock:
             if event_name in self.events:
                 event = self.events[event_name]
-        emitter_name = 'mycroft.event_status.callback.{}'.format(event_name)
+        emitter_name = f'mycroft.event_status.callback.{event_name}'
         self.bus.emit(message.reply(emitter_name, data=event))
 
     def store(self):
@@ -281,19 +280,30 @@ def shutdown(self):
 class EventSchedulerInterface:
     """Interface for accessing the event scheduler over the message bus."""
 
-    def __init__(self, name, sched_id=None, bus=None):
-        self.name = name
-        self.sched_id = sched_id or self.__class__.__name__
+    def __init__(self, name=None, sched_id=None, bus=None):
+        self.name = name or self.__class__.__name__
+        # we can not rename the sched_id keyword argument
+        # because of backwards compat with upstream
+        # renaming it to skill_id would be more accurate
+        self.sched_id = sched_id or self.name
         self.bus = bus
         self.events = EventContainer(bus)
-
         self.scheduled_repeats = []
 
+    @property
+    def skill_id(self):
+        # self.skill_id is preferred usage
+        # self.sched_id can not be deprecated
+        return self.sched_id
+
     def set_bus(self, bus):
         self.bus = bus
         self.events.set_bus(bus)
 
     def set_id(self, sched_id):
+        # we can not rename the sched_id keyword argument
+        # because of backwards compat with upstream
+        # renaming it to skill_id would be more accurate
         self.sched_id = sched_id
 
     def _create_unique_name(self, name):
@@ -306,7 +316,7 @@ def _create_unique_name(self, name):
         Returns:
             str: name unique to this skill
         """
-        return str(self.sched_id) + ':' + (name or '')
+        return self.skill_id + ':' + (name or self.name)
 
     def _schedule_event(self, handler, when, data, name,
                         repeat_interval=None, context=None):
@@ -329,16 +339,15 @@ def _schedule_event(self, handler, when, data, name,
         if isinstance(when, (int, float)) and when >= 0:
             when = datetime.now() + timedelta(seconds=when)
         if not name:
-            name = self.name + handler.__name__
+            name = self.skill_id + handler.__name__
         unique_name = self._create_unique_name(name)
         if repeat_interval:
             self.scheduled_repeats.append(name)  # store "friendly name"
 
         data = data or {}
 
         def on_error(e):
-            LOG.exception('An error occurred executing the scheduled event '
-                          '{}'.format(repr(e)))
+            LOG.exception(f'An error occurred executing the scheduled event {e}')
 
         wrapped = create_basic_wrapper(handler, on_error)
         self.events.add(unique_name, wrapped, once=not repeat_interval)
@@ -347,7 +356,7 @@ def on_error(e):
                       'repeat': repeat_interval,
                       'data': data}
         context = context or {}
-        context["skill_id"] = self.sched_id
+        context["skill_id"] = self.skill_id
         self.bus.emit(Message('mycroft.scheduler.schedule_event',
                               data=event_data, context=context))
 
@@ -410,7 +419,7 @@ def update_scheduled_event(self, name, data=None):
             'data': data
         }
         self.bus.emit(Message('mycroft.schedule.update_event',
-                              data=data, context={"skill_id": self.sched_id}))
+                              data=data, context={"skill_id": self.skill_id}))
 
     def cancel_scheduled_event(self, name):
         """Cancel a pending event. The event will no longer be scheduled
@@ -426,7 +435,7 @@ def cancel_scheduled_event(self, name):
         if self.events.remove(unique_name):
             self.bus.emit(Message('mycroft.scheduler.remove_event',
                                   data=data,
-                                  context={"skill_id": self.sched_id}))
+                                  context={"skill_id": self.skill_id}))
 
     def get_scheduled_event_status(self, name):
         """Get scheduled event data and return the amount of time left
@@ -443,9 +452,9 @@ def get_scheduled_event_status(self, name):
         event_name = self._create_unique_name(name)
         data = {'name': event_name}
 
-        reply_name = 'mycroft.event_status.callback.{}'.format(event_name)
+        reply_name = f'mycroft.event_status.callback.{event_name}'
         msg = Message('mycroft.scheduler.get_event', data=data,
-                      context={"skill_id": self.sched_id})
+                      context={"skill_id": self.skill_id})
         status = self.bus.wait_for_response(msg, reply_type=reply_name)
 
         if status:

mycroft/skills/mycroft_skill/mycroft_skill.py

@@ -137,7 +137,7 @@ def __init__(self, name=None, bus=None, use_settings=True, skill_id=""):
         self.gui = SkillGUI(self)
 
         self._bus = None
-        self._enclosure = None
+        self._enclosure = EnclosureAPI(bus)
 
         #: Mycroft global configuration. (dict)
         self.config_core = Configuration.get()
@@ -163,14 +163,14 @@ def __init__(self, name=None, bus=None, use_settings=True, skill_id=""):
         # fully initialized when self.skill_id is set
         self.file_system = FileSystemAccess(get_temp_path(self.name))
 
-        self.log = LOG.create_logger(self.name)  #: Skill logger instance
+        self.log = LOG
         self.reload_skill = True  #: allow reloading (default True)
 
         self.events = EventContainer(bus)
         self.voc_match_cache = {}
 
         # Delegator classes
-        self.event_scheduler = EventSchedulerInterface(self.name)
+        self.event_scheduler = EventSchedulerInterface()
         self.intent_service = IntentServiceInterface()
 
         # Skill Public API
@@ -207,6 +207,10 @@ def _startup(self, bus, skill_id=""):
             self.skill_id = skill_id or basename(self.root_dir)
             self._init_settings()
             self._init_filesystem()
+            self.log = LOG.create_logger(self.skill_id)  #: Skill logger instance
+            self.intent_service.set_id(self.skill_id)
+            self.event_scheduler.set_id(self.skill_id)
+            self.enclosure.set_id(self.skill_id)
 
             # initialize anything that depends on the messagebus
             self.bind(bus)
@@ -266,8 +270,7 @@ def _init_settings(self):
         self._initial_settings = copy(self.settings)
 
     def _init_filesystem(self):
-        # TODO start using self.skill_id instead of self.name
-        skill_file_system = FileSystemAccess(join('skills', self.name))
+        skill_file_system = FileSystemAccess(join('skills', self.skill_id))
         if not isdir(skill_file_system.path):
             shutil.move(self.file_system.path, skill_file_system.path)
         else:
@@ -421,10 +424,8 @@ def bind(self, bus):
             self._bus = bus
             self.events.set_bus(bus)
             self.intent_service.set_bus(bus)
-            self.intent_service.set_id(self.skill_id)
             self.event_scheduler.set_bus(bus)
-            self.event_scheduler.set_id(self.skill_id)
-            self._enclosure = EnclosureAPI(bus, self.name)
+            self._enclosure.set_bus(bus)
             self._register_system_event_handlers()
             # Initialize the SkillGui
             self.gui.setup_default_handlers()
@@ -460,14 +461,13 @@ def wrapper(message):
                 name = method.__name__
                 self.public_api[name] = {
                     'help': doc,
-                    'type': '{}.{}'.format(self.skill_id, name),
+                    'type': f'{self.skill_id}.{name}',
                     'func': method
                 }
         for key in self.public_api:
             if ('type' in self.public_api[key] and
                     'func' in self.public_api[key]):
-                LOG.debug('Adding api method: '
-                          '{}'.format(self.public_api[key]['type']))
+                LOG.debug(f"Adding api method: {self.public_api[key]['type']}")
 
                 # remove the function member since it shouldn't be
                 # reused and can't be sent over the messagebus
@@ -476,7 +476,7 @@ def wrapper(message):
                                wrap_method(func))
 
         if self.public_api:
-            self.add_event('{}.public_api'.format(self.skill_id),
+            self.add_event(f'{self.skill_id}.public_api',
                            self._send_public_api)
 
     @property
@@ -518,15 +518,15 @@ def handle_settings_change(self, message):
         if self.settings_meta:
             remote_settings = message.data.get(self.settings_meta.skill_gid)
             if remote_settings is not None:
-                LOG.info('Updating settings for skill ' + self.name)
+                LOG.info('Updating settings for skill ' + self.skill_id)
                 self.settings.update(**remote_settings)
                 self.settings.store()
                 if self.settings_change_callback is not None:
                     self.settings_change_callback()
 
     def detach(self):
         for (name, _) in self.intent_service:
-            name = '{}:{}'.format(self.skill_id, name)
+            name = f'{self.skill_id}:{name}'
             self.intent_service.detach_intent(name)
 
     def initialize(self):
@@ -584,7 +584,7 @@ def activate(self):
         if "skill_id" not in msg.context:
             msg.context["skill_id"] = self.skill_id
         self.bus.emit(msg.forward("intent.service.skills.activate",
-                                  data={"skill_id": self.skill_id}))
+                                  {"skill_id": self.skill_id}))
 
     def deactivate(self):
         """remove skill from active_skill list in intent_service.
@@ -594,7 +594,7 @@ def deactivate(self):
         if "skill_id" not in msg.context:
             msg.context["skill_id"] = self.skill_id
         self.bus.emit(msg.forward(f"intent.service.skills.deactivate",
-                                  data={"skill_id": self.skill_id}))
+                                  {"skill_id": self.skill_id}))
 
     def handle_converse_ack(self, message):
         self.bus.emit(message.reply(
@@ -842,11 +842,8 @@ def ask_selection(self, options, dialog='',
 
         if numeric:
             for idx, opt in enumerate(options):
-                opt_str = "{number}, {option_text}".format(
-                    number=pronounce_number(
-                        idx + 1, self.lang), option_text=opt)
-
-                self.speak(opt_str, wait=True)
+                number = pronounce_number(idx + 1, self.lang)
+                self.speak(f"{number}, {opt}", wait=True)
         else:
             opt_str = join_list(options, "or", lang=self.lang) + "?"
             self.speak(opt_str, wait=True)
@@ -899,8 +896,7 @@ def voc_match(self, utt, voc_filename, lang=None, exact=False):
                                                  voc_filename + '.voc'))
 
             if not voc or not exists(voc):
-                raise FileNotFoundError(
-                    'Could not find {}.voc file'.format(voc_filename))
+                raise FileNotFoundError(f'Could not find {voc_filename}.voc file')
             # load vocab and flatten into a simple list
             vocab = read_vocab_file(voc)
             self.voc_match_cache[cache_key] = list(chain(*vocab))
@@ -923,7 +919,7 @@ def report_metric(self, name, data):
             name (str): Name of metric. Must use only letters and hyphens
             data (dict): JSON dictionary to report. Must be valid JSON
         """
-        report_metric('{}:{}'.format(basename(self.root_dir), name), data)
+        report_metric(f'{self.skill_id}:{name}', data)
 
     def send_email(self, title, body):
         """Send an email to the registered user's email.
@@ -970,12 +966,10 @@ def register_resting_screen(self):
             method = getattr(self, attr_name)
             if hasattr(method, 'resting_handler'):
                 self.resting_name = method.resting_handler
-                self.log.info('Registering resting screen {} for {}.'.format(
-                    method, self.resting_name))
+                self.log.info(f'Registering resting screen {method} for {self.resting_name}.')
 
                 # Register for handling resting screen
-                msg_type = '{}.{}'.format(self.skill_id, 'idle')
-                self.add_event(msg_type, method)
+                self.add_event(f'{self.skill_id}.idle', method)
                 # Register handler for resting screen collect message
                 self.add_event('mycroft.mark2.collect_idle',
                                self._handle_collect_resting)
@@ -1285,7 +1279,7 @@ def register_intent_file(self, intent_file, handler):
             name = f'{self.skill_id}:{intent_file}'
             filename = self.find_resource(intent_file, 'vocab', lang=lang)
             if not filename:
-                self.log.error('Unable to find "{}"'.format(intent_file))
+                self.log.error(f'Unable to find "{intent_file}"')
                 continue
             self.intent_service.register_padatious_intent(name, filename, lang)
             if handler:
@@ -1317,7 +1311,7 @@ def register_entity_file(self, entity_file):
             if not filename:
                 self.log.error(f'Unable to find "{entity_file}"')
                 continue
-            name = '{}:{}'.format(self.skill_id, entity_file)
+            name = f'{self.skill_id}:{entity_file}'
             self.intent_service.register_padatious_entity(name, filename, lang)
 
     def handle_enable_intent(self, message):
@@ -1356,8 +1350,7 @@ def disable_intent(self, intent_name):
                 self.intent_service.detach_intent(lang_intent_name)
             return True
         else:
-            LOG.error('Could not disable '
-                      '{}, it hasn\'t been registered.'.format(intent_name))
+            LOG.error(f'Could not disable {intent_name}, it hasn\'t been registered.')
             return False
 
     def enable_intent(self, intent_name):
@@ -1376,11 +1369,10 @@ def enable_intent(self, intent_name):
             else:
                 intent.name = intent_name
                 self.register_intent(intent, None)
-            LOG.debug('Enabling intent {}'.format(intent_name))
+            LOG.debug(f'Enabling intent {intent_name}')
             return True
         else:
-            LOG.error('Could not enable '
-                      '{}, it hasn\'t been registered.'.format(intent_name))
+            LOG.error(f'Could not enable {intent_name}, it hasn\'t been registered.')
             return False
 
     def set_context(self, context, word='', origin=''):
@@ -1477,8 +1469,8 @@ def speak(self, utterance, expect_response=False, wait=False, meta=None):
         """
         # registers the skill as being active
         meta = meta or {}
-        meta['skill'] = self.name
-        self.enclosure.register(self.name)
+        meta['skill'] = self.skill_id
+        self.enclosure.register(self.skill_id)
         data = {'utterance': utterance,
                 'expect_response': expect_response,
                 'meta': meta,
@@ -1637,7 +1629,7 @@ def __handle_stop(self, message):
                                             {"skill_id": self.skill_id}))
         except Exception as e:
             LOG.exception(e)
-            LOG.error(f'Failed to stop skill: {self.name}')
+            LOG.error(f'Failed to stop skill: {self.skill_id}')
 
     def stop(self):
         """Optional method implemented by subclass."""
@@ -1675,14 +1667,12 @@ def default_shutdown(self):
         try:
             self.stop()
         except Exception:
-            LOG.error('Failed to stop skill: {}'.format(self.name),
-                      exc_info=True)
+            LOG.error(f'Failed to stop skill: {self.skill_id}', exc_info=True)
 
         try:
             self.shutdown()
         except Exception as e:
-            LOG.error('Skill specific shutdown function encountered '
-                      'an error: {}'.format(repr(e)))
+            LOG.error(f'Skill specific shutdown function encountered an error: {e}')
 
         self.bus.emit(
             Message('detach_skill', {'skill_id': str(self.skill_id) + ':'},