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

[Config] Group.__call__() has same behaviour as Group.all() #2018

Merged
merged 12 commits into from Aug 26, 2018

Conversation

Tobotimus
Copy link
Member

Type

  • Bugfix

Description of the changes

This modifies config.Group.__call__ to now have the same behaviour of config.Group.all(), and in fact all() is now simply an alias for __call__. Both of these functions are able to be used as a context manager as well.

Furthermore, to match the new behaviour of __call__, get_raw will mix in defaults when a sub-group is being retrieved by it.

This makes config.Group.__call__ effectively an alias for Group.all(),
with the added bonus of becoming a context manager.

get_raw has been updated as well to reflect the new behaviour of
__call__.
@Tobotimus
Copy link
Member Author

Notes about breaking changes

The situations where these changes could be breaking are where you are retrieving data whose default type is a dict, by either calling a config.Group object, or using get_raw() to retrieve a dict. Previously, these functions would not return defaults mixed into the returned dict - the new behaviour is that they do.

As an example:

# old behaviour

config.register_global(foo={'bar': True})
await config.foo()  # -> {}

# new behaviour

config.register_global(foo={'bar': True})
await config.foo()  # -> {'bar': True}

And an example with get_raw():

# old behaviour

config.register_global(foo={'bar': True, 'baz': False})
await config.foo.baz.set(True)
await config.get_raw('foo')  # -> {'baz': True}

# new behaviour

config.register_global(foo={'bar': True, 'baz': False})
await config.foo.baz.set(True)
await config.get_raw('foo')  # -> {'bar': True, 'baz': True}

Concerns

If you have any concerns about how you will update to account for these changes, let us know (in #coding perhaps) and we will do our best to assist you.

@mikeshardmind
Copy link
Contributor

A slight clarification, this is only a breaking change in those cases if you were using that behavior as a method for checking that no value was set. In most cases, registering a default should indicate that you have a default value and do not care if one is not already set, however, if you do care, there are a few ways to handle this.

  • Sentinel Values which will never exist in normal operation as defaults. (None for a value which can't normally be None as an example)
  • Checking the all_members, all_roles, all_guilds (etc) method for their existence.

Copy link
Contributor

@mikeshardmind mikeshardmind left a comment

Choose a reason for hiding this comment

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

This looks solid and appears to be behaving with the new intended behavior. I've left a comment about a particular refactor option in the bank, but it's not a necessity to change the way this was handled, just an option for discussion.

@@ -400,19 +400,18 @@ def _invalid_amount(amount: int) -> bool:

"""
if await is_global():
acc_data = (await _conf.user(member)()).copy()
default = _DEFAULT_USER.copy()
all_accounts = await _conf.all_users()
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make more sense to use a context manager with a sentinel value default for account creation which would be impossible? It's not a required change, but it should simplify the new user case significantly.

Copy link
Member Author

@Tobotimus Tobotimus Aug 17, 2018

Choose a reason for hiding this comment

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

That's true. For now this works so I'm going to leave it as it is, however sentinel values are possible as defaults... You just need to be careful whether you specify a sentinel value as an empty dict, or None.

When using a NoneType sentinel value, that value will not be treated as a config.Group, even if a dict later replaces it. Using an empty dict instead, the value will be treated as a config.Group, and programmers can utilise the extra methods of this object.

@Tobotimus Tobotimus added QA: Passed Used by few QA members. Has been approved by the assigned QA member(s). Breaking Change Will potentially break some cogs. and removed QA: Needed labels Aug 17, 2018
@Tobotimus Tobotimus changed the title [V3 Config] Group.__call__() has same behaviour as Group.all() [Config] Group.__call__() has same behaviour as Group.all() Aug 25, 2018
@Tobotimus Tobotimus merged commit dbed24a into Cog-Creators:V3/develop Aug 26, 2018
@Tobotimus Tobotimus deleted the V3/config_group_call branch August 26, 2018 13:30
@Tobotimus Tobotimus mentioned this pull request Aug 26, 2018
3 tasks
@Jackenmen Jackenmen added the Type: Bug Unexpected behavior, result, or exception. In case of PRs, it is a fix for the foregoing. label Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Will potentially break some cogs. QA: Passed Used by few QA members. Has been approved by the assigned QA member(s). Type: Bug Unexpected behavior, result, or exception. In case of PRs, it is a fix for the foregoing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants