Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update to pyee 8.1.0 #2724

Merged
merged 2 commits into from Oct 22, 2020
Merged

Update to pyee 8.1.0 #2724

merged 2 commits into from Oct 22, 2020

Conversation

forslund
Copy link
Collaborator

Description

pyee 8.1.0 adds a small change to make the once call to be more safe in
multithreaded environments.

This switches back from the now deprecated BaseEventEmitter to the
standard EventEmitter.

Before this PR can be merged Adapt PR #117 also needs to be merged to sync the requirements.

The update should a tleast improve on the behaviour reported in #2695 if not fix it all together.

How to test

Ensure that unit tests and integration tests succeeds.

Contributor license agreement signed?

CLA [ Yes ]

@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Oct 15, 2020
@forslund forslund added Status: Work in progress PR being actively worked on, not yet ready for review. and removed CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) labels Oct 15, 2020
@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Oct 15, 2020
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@krisgesling
Copy link
Contributor

That's awesome, thanks for chasing this upstream!

@j1nx
Copy link

j1nx commented Oct 16, 2020

Don't forget to bump the requirement in mycroft-messagebus-client as well.

@forslund
Copy link
Collaborator Author

Thanks for the reminder @j1nx!

@krisgesling krisgesling added this to Inbox in Roadmap via automation Oct 21, 2020
@krisgesling krisgesling moved this from Inbox to Next in Roadmap Oct 21, 2020
@krisgesling
Copy link
Contributor

Hey Ake, I wanted to check on the WIP label.

Is that because we need to merge the other updates also or are you still testing this?

@krisgesling krisgesling added the Type: Bug - quick Bug fixes that are quick to review and the implications of the change are clear and contained. label Oct 21, 2020
@forslund
Copy link
Collaborator Author

Yes I labled it WIP since it needs Adapt to be updated to match before merging this one. The messagebus change is not mandatory.

@forslund forslund removed the Status: Work in progress PR being actively worked on, not yet ready for review. label Oct 22, 2020
@forslund
Copy link
Collaborator Author

Adapt version has been poked, now the pyee versions of core and adapt is synced.

@krisgesling
Copy link
Contributor

Hey I just noticed there's still a BaseEventEmitter in the wake word tests:
https://github.com/forslund/mycroft-core/blob/feature/pyee-8.1.0/test/wake_word/wake_word_test.py#L103

Assuming this just got missed or is there a specific reason for it remaining?

@forslund
Copy link
Collaborator Author

Good eyes, that's an omission on my part. Didn't search the test folder apparently. Will update

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund
Copy link
Collaborator Author

Updated fixing the wake_word test. (I noticed we/I seem to have broken the wake_word test. I only rarely get successes...will open an issue for this.)

@forslund
Update to pyee 8.1.0
0a9de0a 
pyee 8.1.0 adds a small change to make the once call to be more safe in
multithreaded environments.

This switches back from the now deprecated BaseEventEmitter to the
standard EventEmitter.
@forslund
Update adapt to a version with matching pyee
c04b6bb
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

krisgesling
krisgesling approved these changes Oct 22, 2020
View reviewed changes
Copy link
Contributor

@krisgesling krisgesling left a comment

Choose a reason for hiding this comment

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

🎉

@krisgesling krisgesling merged commit 23f8c3f into MycroftAI:dev Oct 22, 2020
Roadmap automation moved this from Next to Done Oct 22, 2020
@forslund forslund deleted the feature/pyee-8.1.0 branch October 22, 2020 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krisgesling krisgesling krisgesling approved these changes

Assignees
No one assigned
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Type: Bug - quick Bug fixes that are quick to review and the implications of the change are clear and contained.
Projects
Roadmap
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@forslund @devops-mycroft @krisgesling @j1nx @devs-mycroft