config.types, test: add BooleanAttribute setting type

[dgw]

Description

Was going to move the helper functions used in ValidatedAttribute when parse == bool, but it would've required some ridiculous polymorphism. Better to just leave the old stuff—which will also make it easier to mark as deprecated.

Maybe we need to add tests that cover the default parameter to some or all of these setting types? I'll chew on that.

When complete, this will implement and close #2019.

Checklist

• I have read CONTRIBUTING.md
• I can and do license this contribution under the EFLv2
• No issues are reported by make qa (runs make quality and make test)
• I have tested the functionality of the things this change touches

[lgtm-com]

This pull request introduces 1 alert when merging 6fb5c0a into 4639e00 - view on LGTM.com

new alerts:

• 1 for Missing call to `__init__` during object initialization

[dgw]

Simply could not think of a better approach to emitting deprecation warnings for ValidatedAttribute(..., parse=bool). Obviously decorating ValidatedAttribute.__init__() as @deprecated would cover a LOT more cases than just the Boolean ones. Decorating the helpers _parse_boolean() and _serialize_boolean() caused tons of spam in the terminal. 🤷‍♂️

[Exirel]

Decorating the helpers _parse_boolean() and _serialize_boolean() caused tons of spam in the terminal. man_shrugging

Indeed. If you want to reduce the duplicated code, you can:

1. put everything from the if bool block into a function/a method
2. put the deprecation warning on that function/method
3. call it in the if bool and nowhere else

The content of said function would be something like:

@deprecate(...)
def _get_deprecated_boolean_handlers(parser=None, serializer=None):
    # ... logic from that if bool block
    return the_boolean_parser, the_boolean_serializer

Problem solved.

[dgw]

Problem solved.

Not really. I mean technically, it deduplicates the code, but the warning printed is this:

Deprecated since 7.1, will be removed in 9.0: Use BooleanAttribute instead of ValidatedAttribute with parse=bool
  File "/mnt/c/Users/dgw/github/sopel/sopel/config/types.py", line 317, in __init__
    parse, serialize = _deprecated_special_bool_handling(serialize)

That doesn't tell the user or plugin developer where the outdated usage lies.

If tools.deprecated() had a kwarg to specify which stack frame to show, that might help, but it's way out of scope for this PR. That said, I already put together the bare bones of such a new kwarg; I'd just need to document it to make it PR-ready.

[Exirel]

We can always add the field's name in the deprecation message, that should help a bit.

[dgw]

I'm not at all sure how you'd do that from a decorator on a helper function called from the object constructor, but if I can learn something from you again, I'm all eyes.

[Exirel]

I'd do that within the constructor:

def __init__(self, name, ...):
    @deprecate("some message with the name %s" % name)
    def _get_deprecated_boolean_handlers(parser=None, serializer=None):
        # ... logic from that if bool block
        return the_boolean_parser, the_boolean_serializer

    if bool:
        parser, serialiazer = _get_deprecated_boolean_handlers()

Not perfect, but it works and it's temporary.

[dgw]

I guess defining stuff inside other defs is not unheard of in Sopel's code, e.g. the old signal handlers. But even that approach would benefit from the PR I'm about to open against tools.deprecated.

And don't worry, I did see your muttering on IRC about using warnings.warn(). That's something tools.deprecated could do instead of directly printing to stderr itself. I've also mused recently that that decorator should output to the logs along with everything else. Lots to think about for 8.0!

[Exirel]

even that approach would benefit from the PR

Yup, and if that PR can be merged first, go for it. I think it would be the better option here.

[dgw]

Pushed using the new kwarg to tools.deprecated. If this doesn't pass CI I'll do a rebase later.

Might want to clean up history later anyway, tbf.

[lgtm-com]

This pull request introduces 1 alert when merging bed0471 into 96c55af - view on LGTM.com

new alerts:

• 1 for Unused import

[dgw]

I cleaned up the history. @Exirel presumably will be the one to review this, as it uses his suggested approach.

[Exirel]

👍