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.

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

Use a metaclass for config's singletons #3137

Merged
merged 2 commits into from Feb 28, 2020
Merged

Use a metaclass for config's singletons #3137

merged 2 commits into from Feb 28, 2020

Conversation

mikeshardmind
Copy link
Contributor

@mikeshardmind mikeshardmind commented Nov 18, 2019

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

This is technically a fix. This closes #3136

#3136 isn't what we had in mind when handling config as a singleton (and we don't intend for people to think accessing other cog's data should be done without that cog being designed for it), but we should be preventing reinitialization of existing config objects from this.

This also removes the potential for an error to be raised in a safe case of using init_group multiple times in the same config object, allowing multiple cogs to more safely share a config object if related.

It will still error if you try to change the identifier count of a group as this would be unsafe.

@mikeshardmind
Copy link
Contributor Author

@mikeshardmind mikeshardmind commented Jan 3, 2020

This one shouldn't be merged without @tekulvw signing off on it. It's not a release blocker, and will be moved out of the 3.2 milestone should it not be ready in time.

@mikeshardmind mikeshardmind removed this from the 3.2.0 milestone Jan 4, 2020
@mikeshardmind mikeshardmind added this to the 3.2.1 milestone Jan 4, 2020
Copy link
Member

@TrustyJAID TrustyJAID left a comment

This fixes an issue I had when using get_conf multiple times within my own cog.

@mikeshardmind mikeshardmind removed this from the Next Feature release milestone Jan 12, 2020
@jack1142 jack1142 added the Category: Config label Jan 17, 2020
@mikeshardmind mikeshardmind deleted the 3136-fix branch Jan 27, 2020
@mikeshardmind mikeshardmind restored the 3136-fix branch Jan 27, 2020
@mikeshardmind
Copy link
Contributor Author

@mikeshardmind mikeshardmind commented Jan 27, 2020

RIP, wrong branch...

@mikeshardmind mikeshardmind reopened this Jan 27, 2020
@mikeshardmind mikeshardmind added this to the 3.3.2 milestone Feb 13, 2020
@mikeshardmind
Copy link
Contributor Author

@mikeshardmind mikeshardmind commented Feb 13, 2020

It's been a few months waiting for a potential yes/no out of respect for the initial long discussion in discord about this not using a metaclass.

As this fixes an issue people are currently experiencing and there has been no response, I'm unwilling to hold this longer.

@mikeshardmind
Copy link
Contributor Author

@mikeshardmind mikeshardmind commented Feb 19, 2020

This needs an additional modification beyond this prior to merge, will handle tommorow.

@mikeshardmind mikeshardmind added the Blocked By: Damage Control label Feb 19, 2020
@mikeshardmind mikeshardmind removed the Blocked By: Damage Control label Feb 21, 2020
@mikeshardmind
Copy link
Contributor Author

@mikeshardmind mikeshardmind commented Feb 21, 2020

The other potential failure related to shared config access is patched now too.

With these changes, it's still unsafe to interact with config objects you don't fully control for both practical purposes and synchronization reasons, but config objects can be safely shared if the various sync concerns related to this are handled.

Copy link
Member

@jack1142 jack1142 left a comment

LGTM

@jack1142 jack1142 changed the title Usea metaclass for config's singletons Use a metaclass for config's singletons Feb 28, 2020
@jack1142 jack1142 merged commit f636199 into Cog-Creators:V3/develop Feb 28, 2020
5 checks passed
@mikeshardmind mikeshardmind deleted the 3136-fix branch Feb 29, 2020
@jack1142 jack1142 added the Type: Bug label Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Config Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants