admin: add unset command

[HumorBaby]

If the value to be set is ever literally 'None' (string), then the setting is set to actual NoneType None. When a true None is set to a config attribute, the setting will essentially be reset to the default value. I feel this should be the expected behavior; i.e., a user should not be able to set a null value for something with a default, because presumably, that default is there for a reason.

---

Testing: (see #1554 for steps)

# admin_set_test.py
...
class AdminSetTestSection(StaticSection):
    test_setting = ValidatedAttribute('test_setting', default=None)
    test_list = ListAttribute('test_list', default=['a','b','c'])
...
@sopel.module._privmsg
@sopel.module.commands('ast-show')
def show(bot, trigger):
    bot.say('repr: "{}", type: "{}"'.format(
        repr(bot.config.admin_set_test.test_setting),
        type(bot.config.admin_set_test.test_setting)
    ))
    bot.say('repr: "{}", type: "{}"'.format(
        repr(bot.config.admin_set_test.test_list),
        type(bot.config.admin_set_test.test_list)
    ))

# _See_ current setting value
<HumorBaby>  .set admin_set_test.test_setting
<testbot>  HumorBaby: admin_set_test.test_setting = None

# See repr and type of setting
<HumorBaby>  .ast-show
<testbot>  repr: "None", type: "<type 'NoneType'>"

# Set a random value to setting
<HumorBaby>  .set admin_set_test.test_setting abc123

# Confirm change
<HumorBaby>  .set admin_set_test.test_setting
<testbot>  HumorBaby: admin_set_test.test_setting = abc123
<HumorBaby>  .ast-show
<testbot>  repr: "u'abc123'", type: "<type 'unicode'>"

# Try to "delete"/unset setting
<HumorBaby>  .set admin_set_test.test_setting None

# Confirm change
<HumorBaby>  .set admin_set_test.test_setting
<testbot>  HumorBaby: admin_set_test.test_setting = None
# everything looks okay...

# It is!
<HumorBaby>  .ast-show
<testbot>  repr: "None", type: "<type 'NoneType'>"

# Test the `ListAttribute` type
<HumorBaby>  .set admin_set_test.test_list
<testbot>  HumorBaby: admin_set_test.test_list = [u'a', u'b', u'c']
<HumorBaby>  .ast-show
<testbot>  repr: "[u'a', u'b', u'c']", type: "<type 'list'>"

# Change the list
<HumorBaby>  .set admin_set_test.test_list 1,2,3
<HumorBaby>  .set admin_set_test.test_list
<testbot>  HumorBaby: admin_set_test.test_list = [u'1', u'2', u'3']
<HumorBaby>  .ast-show
<testbot>  repr: "[u'1', u'2', u'3']", type: "<type 'list'>"

# Unset the list
<HumorBaby>  .set admin_set_test.test_list None
<HumorBaby>  .set admin_set_test.test_list
<testbot>  HumorBaby: admin_set_test.test_list = [u'a', u'b', u'c']
<HumorBaby>  .ast-show
<testbot>  repr: "[u'a', u'b', u'c']", type: "<type 'list'>"

Fixes #1554

[Exirel]

Issue #1554 raises valid concern about how .set work, and the lack of "unset a config value". However, what you do here is a hack that relies on the interpretation of a string that may or may not be what the user actually wants. So I'm not on board with this solution.

IMHO, I'd rather have .unset section.option to remove the option and .reset section.option to set its value to its default value, and no way to set it to None (as-in the NoneType object None which make no sense for a configuration file that is all about strings).

[HumorBaby]

However, what you do here is a hack that relies on the interpretation of a string that may or may not be what the user actually wants. So I'm not on board with this solution.

I'm with you completely. I felt a little uneasy when I was doing it, but somehow I ignore my instincts and convinced myself it was okay 😅

no way to set it to None (as-in the NoneType object None which make no sense for a configuration file that is all about strings).

Yes, I agree 😄 If a string None is present in the config, then it will be interpreted as a string, as expected. Thus, it makes no sense to try to set a NoneType None to a configuration setting and expecting it to be written/saved that way.

Also, given how setting values are pulled from a config object, just means that a None value will return the default for that setting. If a truly None value is desired, then it must be the default for that item. If there is a non-None default, and a user wants to set a None value to the setting, it will not be possible to save that value to config. Another point: I cannot think of a setting that would need to be nullified when the default value is non-None. Overall, this relates to your point:

.unset section.option to remove the option and .reset section.option to set its value to its default value

There is no real meaning to having removed an option. It will just return the default. And because setting a value to None will also return the default, I don't see a reason to have .unset and .reset as they will have the same result. I think .unset is the better option to keep here, just because it is more parallel to the .set terminology.

[HumorBaby]

Since this is a relatively large change... changing to master, but not rebasing to hopefully avoid conflicts once 6.6.x is merged in.

[dgw]

I definitely think adding an .unset command is the way to go, but handling both commands in the same function (and defining all the logic to manipulate the config inside that function) is… kinda ew.

Not that I have any modules that look at trigger.group(1) to see how they're being called… but they're much simpler. This gets pretty complex by trying to do everything in one place. 😅

[HumorBaby]

but handling both commands in the same function (and defining all the logic to manipulate the config inside that function) is… kinda ew.

Yea... was trying to minimize duplication of the section and option parsing. Refactor coming soon, then.

[Exirel]

Refactor coming soon, then.

That's the way to go! 👍

[HumorBaby]

Refactor coming soon.

'tis here.

[dgw]

I guess I'll come back to this in a few days, after 6.6.x is merged into master and you can rebase. 😁

[dgw]

Ready to rebase; I merged 6.6.x into master just now.

[HumorBaby]

🎉

[Exirel]

Control flow is key!

[Exirel]

Almost done. 👍

[dgw]

I think this is at nitpick stage. 🎉

[dgw]

Nitpicks have been addressed! 🎉 Now there's just a squash (of the fixup commits) and a rebase (to resolve the conflicts introduced on master) before final review & approval. 👍

[HumorBaby]

Now there's just a squash (of the fixup commits) and a rebase (to resolve the conflicts introduced on master) before final review & approval. 👍

Done.

[dgw]

🎉 Thanks for the rebase!

[Exirel]

Looks good to me.

I've a small question about how to display the InvalidSection and InvalidSectionOption, you can try and see if it can be better, or you can ignore it, it's fine by me.