config: Warn if unexpected settings exist

[dgw]

When loading the config file, Sopel should warn the user if it finds a setting name that isn't defined in the StaticSection for that section. (Obviously, this logic will have to ignore the per-channel config sections added in #1235—or handle those, too, since they also have clearly defined properties.)

Such a warning should not prevent starting the bot or halt/block loading the module to whom the setting belongs; it should be purely informational.

This would very much help users avoid typos after manually editing their config files, and aid in debugging issues like the one @HumorBaby and I just spent literally hours on—which turned out to be the result of a misnamed setting in the .cfg file Sopel was using. (The fact that the incorrect name made more sense than the correct one is a bug for another repo.)

[HumorBaby]

(Obviously, this logic will have to ignore the per-channel config sections added in #1235—or handle those, too, since they also have clearly defined properties.)

It could just validate StaticSections only. Since the per-channel config sections are not of that type, it wouldn't be thrown off by those 😄 Just adding my thoughts... not that I had anything else to add about the actual implementation itself 😛 I was just reminded of this because the admin module explicitly checks for whether a section is StaticSection, and it took me forever to realize why such a check was even needed... for cases like the per-channel config (i.e., non-module config)!

[dgw]

Huh, actually that StaticSection type check came from a813671, which fixed an old bug I reported (and introduced a new bug that I was surprised Python itself wouldn't complain about at runtime… see if you can spot it in the diff before looking at the fix in 111fc5c).

It has nothing to do with the per-channel stuff, and everything to do with the fact that, pre-6.0, the config sections were lawless lands that could contain whatever you might toss in. Sopel 6 brought validation of config attributes and the StaticSection definitions we know and love today. My guess is the error handling you found is for legacy support of some weird use cases that rely on having arbitrary config values not defined in a section prototype, I dunno.

All of that is by way of saying, cool, whoever implements this can just do the same kind of check. 😁

[HumorBaby]

(and introduced a new bug that I was surprised Python itself wouldn't complain about at runtime… see if you can spot it in the diff before looking at the fix in 111fc5c).

...introduced this control-flow one too that has been hanging around this whole time! 😅

sopel/willie/modules/admin.py

Lines 198 to 200 in a813671

	if not static_sec and bot.config.parser.has_option(section, option):
	bot.reply("Option %s.%s does not exist." % (section_name, option))
	return

It has nothing to do with the per-channel stuff, and everything to do with the fact that, pre-6.0, the config sections were lawless lands that could contain whatever you might toss in ... My guess is the error handling you found is for legacy support of some weird use cases that rely on having arbitrary config values not defined in a section prototype

... figured it was something like that, since of course per-channel config didn't exist 😁 I got about 5-6 years back in the revision history before I gave up trying to put it all together. Regardless, I'm glad it stuck around and now can directly apply!

[dgw]

That's a nice control flow bug. Parentheses are useful, hmm?

It's close to the password-censoring logic, too, which (after our adventure yesterday) should probably also censor any option ending in "secret". Off topic, but hey—I know you like making little PRs. 😝

[HumorBaby]

Off topic, but hey—I know you like making little PRs

It's a serious problem 😬

That's a nice control flow bug. Parentheses are useful, hmm?

@Exirel pointed it out in review for #1556 😄 so the fix is in there. Let me know if it should go in a smaller PR for merge sooner...

should probably also censor any option ending in "secret".

Okay. But what about settings that don't endswith "secret" (e.g., "secret_codeword") or "password"... is in may be better no? Better safe than sorry...

The other option is to add a boolean attribute to config.types.BaseValidated called is_secret or something and only display values to the owner (or not at all)...? Which would obviously be a much larger change, wrt the API, and may not be worth it.

[HumorBaby]

The other option is to add a boolean attribute to config.types.BaseValidated called is_secret or something and only display values to the owner (or not at all)...? Which would obviously be a much larger change, wrt the API, and may not be worth it.

probably overkill just for admin.py...

[dgw]

Pretty definitely overkill. We should take further discussion of that to a new issue, though, if it needs bikeshedding (and it seems to).

[dgw]

This feature is a fine candidate for a minor release, and doesn't need to block 7.0.

[dgw]

@Exirel This would definitely help users out a lot when configuring plugins, e.g. if they mistype a setting name. Yes, we can encourage the use of sopel-plugins configure, but supporting that isn't required for plugins. Seems not too invasive.

[Exirel]

So, I've something in progress for that, but I need #1933 to be merged to prevent possible merge conflict with the test directory.