MycroftAI / adapt

Adapt Intent Parser
Apache License 2.0
711 stars 155 forks source link

Possible bug in context merging in context.py #97

Closed ghost closed 3 years ago

ghost commented 4 years ago

I'm new to mycroft, and I started to study the codebase (great stuff!). I noticed in Adapt this bug:https://github.com/MycroftAI/adapt/blob/7eeadeb4744b7e2dd7a9aa61e0350c4e22350eba/adapt/context.py#L73-L88

The keys of metadata are used instead of the values in merging. I believe it should be something like

    def merge_context(self, tag, metadata):
        """
        merge into contextManagerFrame new entity and metadata.

        Appends tag as new entity and adds keys in metadata to keys in
        self.metadata.

        Args:
            tag(str): entity to be added to self.entities
            metadata(object): metadata containes keys to be added to self.metadata
        """
        self.entities.append(tag)
        for k, v in metadata.items():
            if k not in self.metadata:
                self.metadata[k] = v

I'm still not very familiar with the codebase, so sorry if I got this wrong.

clusterfudge commented 3 years ago

Hey @theartful , thanks for the issue! It looks like we don't have any test coverage or users of this, so good eye!