Skip to content
This repository has been archived by the owner on Sep 8, 2024. It is now read-only.

Feature/gui get #2820

Merged
merged 2 commits into from
Feb 5, 2021
Merged

Feature/gui get #2820

merged 2 commits into from
Feb 5, 2021

Conversation

krisgesling
Copy link
Contributor

Description

Currently if a Skill attempts to access a value on the GUI interface that doesn't exist a KeyError will be raised.

This implements the Pythonic behaviour of a dict.get() method to provide a safer mechanism for accessing those keys including an optional parameter for returning a default value.

While there, I added some extra unittests for the SkillGUI class.

How to test

Unit tests included. Or you can drop the following into a Skills initialize method

        self.gui['some'] = 'value'
        self.log.info(self.gui['some'])
        self.log.info(self.gui.get('thing', "WORKING"))
        self.log.info(self.gui['thing'])  # Raises KeyError

Contributor license agreement signed?

@krisgesling krisgesling added Type: Enhancement - proposed New proposal for a feature that is not currently a priority on the roadmap. Status: To be reviewed Concept accepted and PR has sufficient information for full review Type: Tests Addition or improvement of automated tests labels Feb 2, 2021
@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Feb 2, 2021
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund
Copy link
Collaborator

forslund commented Feb 2, 2021

Looks good. For some history, this was originally left out to make the class as different as possible to the standard dict since we didn't wish to implement it fully. For example it has no __in__() or iterator methods and merge operations can't be performed, etc.

I would encourage to also add at least __in__() and possibly the iter methods.

edit:
in is implemented it's __contains__() not __in__(), My bad

@@ -110,6 +110,14 @@ def __getitem__(self, key):
"""Implements get part of dict-like behaviour with named keys."""
return self.__session_data[key]

def get(self, key, value=None):
"""Implements the get method for accessing dict keys."""
try:
Copy link
Collaborator

@forslund forslund Feb 2, 2021

Choose a reason for hiding this comment

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

Suggestion:

Instead of this implementation just forward to __session_data.get

    def get(self, *args, **kwargs):
        return self.__session_data.get(*args, **kwargs)

@krisgesling
Copy link
Contributor Author

I changed to your suggested implementation and thought of adding __iter__ however then I started wondering about keys, items, values, pop, etc.

So I thought it worth deciding what we really want here and whether it would be easier to inherit from dict rather than re-adding everything or if that will introduce it's own strange behaviour.

@krisgesling krisgesling added Status: For discussion Feature proposal in development. Community input and discussion is invited. and removed Status: To be reviewed Concept accepted and PR has sufficient information for full review labels Feb 4, 2021
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@forslund
Copy link
Collaborator

forslund commented Feb 4, 2021

Indeed, this was what we thought when we created it. Limit it to the basics and then if needed make it a subclass of dict.

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@krisgesling krisgesling merged commit b1be522 into dev Feb 5, 2021
@krisgesling krisgesling deleted the feature/gui-get branch February 5, 2021 05:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Status: For discussion Feature proposal in development. Community input and discussion is invited. Type: Enhancement - proposed New proposal for a feature that is not currently a priority on the roadmap. Type: Tests Addition or improvement of automated tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants