config: small refactoring and unit tests for types

[Exirel]

Description

All I wanted was to add a new type of attribute (something like a FlagAttribute to simplify how boolean attributes are handled in config). But then, I discovered an interface I really don't like.

So instead I wrote some unit-test, refactored some bit. I would like to go further, but I need some approval first.
Also, all these tests are still good and shouldn't go to waste or kept in a forgotten local branch.

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

[dgw]

I've gone through most of this, but could use a bit of background on what you were trying to accomplish with the FilenameAttribute changes. Without context, I just see removing a bunch of stuff that doesn't appear to be replaced with new code doing the same thing. (Looking at magic __set__/__get__ methods and env-var handling, specifically.)

Surely there's a connection between these changes and the ones above in BaseValidated etc., that I'm just not seeing. 🙁 And if that's not the code-review equivalent of "it's not you, it's me", I don't know what is. 😅

Sidebar: Now that FilenameAttribute creating the file/directory it's set to has been shoved in my face, I'm not sure I like that behavior. I'll add that to my list of things to think about, and write up an issue if I still don't like it after investigating.

[Exirel]

I've gone through most of this, but could use a bit of background on what you were trying to accomplish with the FilenameAttribute changes.

Before:

• BaseValidated.parse(value, *args, **kwargs): public, official, sopel.config.types's interface
• BaseValidated.__get__: I'm NOT going to use anything but parse(value)
• ValidatedAttribute.parse(value): I laught at your public interface and do something that is NOT compatible but will force you to deal with my dumb new interface FOREVER
• FilenameAttribute.parse(value, settings, section): lol I'm drunk I don't know what I'm doing (also I'm going to create file & dir in your back)
• same for serialize

After:

• BaseValidated.parse(value, *args, **kwargs): same but we now know that, in reality, it's more parse(value) because of other classes
• BaseValidated._parse(value, settings, section): the hack that allows to mix together the initial interface, the shitty version from ValidatedAttribute, and the drunk one from FilenameAttribute
• FilenameAttribute.parse(value): now you're following the (still shitty) interface set by other classes, now it's unified!
• same for _serialize and serialize

If I could, I'd wreak havoc on BaseValidated.parse/serialize directly, this way I'd not have to care about creating an intermediate layer to make it works. It's your call.

Sidebar: Now that FilenameAttribute creating the file/directory it's set to has been shoved in my face, I'm not sure I like that behavior. I'll add that to my list of things to think about, and write up an issue if I still don't like it after investigating.

Yeah I hate that too, but there are plugins (at least one is built-in IIRC) that expects this behaviour. It annoys me.

This whole config annoys me.

[dgw]

If I could, I'd wreak havoc on BaseValidated.parse/serialize directly, this way I'd not have to care about creating an intermediate layer to make it works. It's your call.

Seems like something that could be changed for… what would you say to 9.0? Though I guess, as usual, it's a trade-off between maintaining backward compatibility with older code that implements custom config types (probably rare) and making the current code more straightforward. Yeah, what else is new. 😁

But honestly, just because implementing custom config setting types seems like such an obscure thing to do, I'm not sure it's worth the effort of making sure warnings about a Grand Interface Change track across documentation and changelogs for multiple Sopel releases. I need more of a cost/benefit analysis than exists right now; "wreak havoc" doesn't tell me much except hint that it'd be a breaking change. 😝 Abstraction layers aren't inherently bad if there's a purpose for them. We do use SQLAlchemy, after all…

Sidebar: Now that FilenameAttribute creating the file/directory it's set to has been shoved in my face, I'm not sure I like that behavior. I'll add that to my list of things to think about, and write up an issue if I still don't like it after investigating.

Yeah I hate that too, but there are plugins (at least one is built-in IIRC) that expects this behaviour. It annoys me.

Maybe for 8.0 we can plan an extra kwarg for FilenameAttribute that can switch off auto-creating the given path? Obviously all existing code is built for the current behavior, so that would have to remain the default (for a while), but new code that doesn't want it could opt out. Shall I make a new issue for that? 😺

[Exirel]

just because implementing custom config setting types seems like such an obscure thing to do

Basically, all I need is to add 2 mandatory parameters to parse and serialize. That would break any subclasses that override these methods without the proper signature, because they would be called with "unknown arguments".

Except, if they do it right, they should have this signature at the moment:

def parse(self, value, *args, **kwargs):

So in theory, I could add the 2 parameters I want. However, since Sopel's own subclasses don't follow this interface, I'm not sure what other people have done. Did they follow the initial interface? Did they follow what Sopel does?

That's what I call "wreak havoc" on that code: making sure the code gets its shit together.

[Exirel]

Oh, here is the main issue I have right now with what Sopel does:

if isinstance(descriptor, types.FilenameAttribute):
    value = descriptor.parse(value, bot.config, descriptor)
else:
    value = descriptor.parse(value)

So here, I've to use my new _parse(value, settings, section) method, because one may assume that parse takes only one argument.

[Exirel]

@dgw any news on that one?

[dgw]

Even after switching my diff view from Unified to Split, I'm still only about 90% sure I understand all the changes. But there's a hefty new test suite to verify behavior, so while you have a chance to address my one actionable nitpick I'll go run the tests on current master and see if anything that shouldn't be broken, is.

Edit: Only the test_filename_parse* tests fail on master, because the signature changed. Good enough for me.

[Exirel]

And done.

[dgw]

I don't know how I keep forgetting about this PR. Let's go, though.