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

Refactor/settings cleanup #2560

Merged
merged 4 commits into from
May 31, 2020
Merged

Conversation

forslund
Copy link
Collaborator

@forslund forslund commented May 2, 2020

Description

Clean up a couple of unused, parts of the settings system.

How to test

Make sure settings are still downloaded and updated on change on home.

Contributor license agreement signed?

CLA [ Yes ]

@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label May 2, 2020
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@MycroftAI MycroftAI deleted a comment from devops-mycroft May 4, 2020
@forslund forslund force-pushed the refactor/settings-cleanup branch from 8ef3437 to 0d1b984 Compare May 8, 2020 07:17
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

Copy link
Member

@chrisveilleux chrisveilleux left a comment

Choose a reason for hiding this comment

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

One minor comment re: formatting

'mycroft.skills.settings.changed',
data={skill_gid: remote_settings}
)
message = Message('mycroft.skills.settings.changed',
Copy link
Member

Choose a reason for hiding this comment

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

This change would be overwritten by Black. You should just run Black on this module, as with any module we check in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok good to know, I'll revert this change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll leave complete blacking out this time since we should probably discuss which settings Black should use before starting to move from pycodestyle approved to black.

Copy link
Member

Choose a reason for hiding this comment

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

The beauty of Black is that there really are no settings. It is overly opinionated so that we don't have to be. I don't agree with all the things Black does, but I put consistently styled code over my objections.

- Update docstrings
- Remove unused members
- Minor logic change to make download method more self-contained
@forslund forslund added the Merge after next release For large changes that look good, but we want to keep in Dev a little longer label May 27, 2020
@MycroftAI MycroftAI deleted a comment from devops-mycroft May 27, 2020
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund forslund merged commit a0c7ea7 into MycroftAI:dev May 31, 2020
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) Merge after next release For large changes that look good, but we want to keep in Dev a little longer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants