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

Add optional message argument #2704

Merged
merged 1 commit into from
Oct 2, 2020
Merged

Conversation

krisgesling
Copy link
Contributor

Description

When called over the messagebus the message is passed to the handler,
thereby throwing a TypeError as it wasn't expecting 2 positional args.

How to test

Ask Mycroft to "update configuration"

Contributor license agreement signed?

@krisgesling krisgesling added Status: To be reviewed Concept accepted and PR has sufficient information for full review Type: Bug - quick Bug fixes that are quick to review and the implications of the change are clear and contained. labels Sep 25, 2020
@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Sep 25, 2020
forslund
forslund previously approved these changes Sep 25, 2020
Copy link
Collaborator

@forslund forslund left a comment

Choose a reason for hiding this comment

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

Nice catch!

Maybe add it to the docstring?

When called over the messagebus the message is passed to the handler,
thereby throwing a TypeError as it wasn't expecting 2 positional args.
@krisgesling
Copy link
Contributor Author

Good call

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

1 similar comment
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund
Copy link
Collaborator

forslund commented Oct 2, 2020

I think this should be merged now as it's very minor and very correct

@krisgesling krisgesling requested a review from forslund October 2, 2020 10:49
@krisgesling
Copy link
Contributor Author

I recently updated the branch protections so that a current "approve" review is needed before merges are allowed into master, and you can't review your own PR. I anticipate it being slightly annoying, but it enforces our standard practice and means the person reviewing has to be 100% clear that they approve the code in its current form to be merged.

@forslund forslund merged commit b241f1d into dev Oct 2, 2020
@forslund forslund deleted the bugfix/settings-update-typeerror branch October 2, 2020 11:40
@krisgesling
Copy link
Contributor Author

Thanks Ake 🙂

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: To be reviewed Concept accepted and PR has sufficient information for full review Type: Bug - quick Bug fixes that are quick to review and the implications of the change are clear and contained.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants