Skip to content

Conversation

@JarbasAl
Copy link
Member

@JarbasAl JarbasAl commented Nov 4, 2021

makes self.config_core a property, ensures we always get the latest mycroft.conf

Configuration is a singleton, keeps an internal cache and reacts to bus events. This does not repeatedly read from disk

this is particularly relevant in the self.location property, once PHAL is implemented it will also solve the associated TODO

it is also relevant for self.lang, the hypothetical future algorithm to switch mycroft language on the fly will need this

relevant snippet from MycroftSkill class

  @property
    def lang(self):
        """Get the current language."""
        lang = self._core_lang
        message = dig_for_message()
        if message:
            lang = message.data.get("lang") or lang
        return lang.lower()

    @property
    def _core_lang(self):
        """Get the configured default language.
        NOTE: this should be public, but since if a skill uses this it wont
        work in regular mycroft-core it was made private! Equivalent PRs in
        mycroft-core have been rejected/abandoned"""
        return self.config_core.get("lang", "en-us").lower()
        
    @property
    def location(self):
        """Get the JSON data struction holding location information."""
        # TODO: Allow Enclosure to override this for devices that
        # contain a GPS.
        return self.config_core.get('location')

    @property
    def location_pretty(self):
        """Get a more 'human' version of the location as a string."""
        loc = self.location
        if type(loc) is dict and loc['city']:
            return loc['city']['name']
        return None

    @property
    def location_timezone(self):
        """Get the timezone code, such as 'America/Los_Angeles'"""
        loc = self.location
        if type(loc) is dict and loc['timezone']:
            return loc['timezone']['code']
        return None

related issues:

makes self.config_core a property, ensures we always get the latest mycroft.conf

Configuration is a singleton, keeps an internal cache and reacts to bus events. This does not repeatedly read from disk
@JarbasAl JarbasAl added the bug Something isn't working label Nov 4, 2021
This was referenced Nov 11, 2021
JarbasAl added a commit that referenced this pull request Nov 26, 2021
makes self.config_core a property, ensures we always get the latest mycroft.conf

Configuration is a singleton, keeps an internal cache and reacts to bus events. This does not repeatedly read from disk
@JarbasAl
Copy link
Member Author

this one stays on hold, testing in alpha release revealed that some downstreams are assigning to some variables that become properties with this PR. this is a breaking change that warrants further discussion.

I still think this is a good change, but if its breaking then maybe its also a chance to further improve some things so it is only a breaking change once....

@JarbasAl JarbasAl marked this pull request as draft January 25, 2022 18:02
@JarbasAl JarbasAl added the on hold blocked until next major release label Jan 25, 2022
Copy link
Member

@NeonDaniel NeonDaniel left a comment

Choose a reason for hiding this comment

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

These changes will require some manual testing on my part (probably after 0.0.2 release). This breaks code that imports and overrides any of these variables that are now properties; following semver, these changes would constitute ovos-core 1.0.0

return config.get("url")

@property
def version(self):
Copy link
Member

Choose a reason for hiding this comment

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

What is this the version of, the local instance or the remote server? (basically asking if remote settings will override this setting)

self._loaded = MonotonicEvent()
self.load_services()

@property
Copy link
Member

Choose a reason for hiding this comment

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

This has the potential of being a breaking change like the same change in MycroftSkill was, though I don't think this one affects Neon

# Create Message Bus Client
self.bus = MessageBusClient()

@property
Copy link
Member

Choose a reason for hiding this comment

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

Same as above; these were exposed variables, so custom enclosures may have overridden them

@ChanceNCounter
Copy link

I will also want to toy with them, but for the other reason! I am very excited about these functions 🤓

@JarbasAl JarbasAl mentioned this pull request Feb 1, 2022
@JarbasAl
Copy link
Member Author

replaced by #105

@JarbasAl JarbasAl closed this Apr 20, 2022
@JarbasAl JarbasAl deleted the fix/mutable_config branch August 8, 2023 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working on hold blocked until next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants