In the current implementation, CommandHandler.command is actually always a List …

Hi. thanks for the PR! IISC so far you have only allowed for the PrefixHandler.prefix/command setters to accept sets. I think I should clarify a bit what the intention of #2885 was:

• The main idea of the issue was to rename CommandHandler.command to CommandHandler.commands (plural) since that attribute is currently actually always a list. Extending this to PrefixHandlers.prefix is surely reasonable.
• Since I noticed that we only ever need x in self.command, my idea was to make CommandHandler.commands a set instead of a list. Accepting sets as input in addition is surely a logical extension. So

Additionally, while reviewing your PR, I noticed that unlike PrefixHandler.command, CommandHandler.command doesn't have a setter. I.e. if you do command_handler.command = 'NewCommand', it won't be converted to [newcommand]. I think we should fix that. So what I would ask you to do is:

• Make sure that sets are accepted as input for Prefix/CommandHandler.command and PrefixHandler.prefix - this includes type hints & documentation
• rename CommandHandler.command to CommandHandler._commands and add getter + setter proprties CommandHandler.commands. The getter should provide access to the set CH._commands and the setter should accept a single string or tuple/list/set and convert it as it is converted on __init__
• Apply analogous changes to PrefixHandler

Thanks for the review, I'm working on it. should I change the name in arguments from command to commands too?

should I change the name in arguments from command to commands too?

no need IMO, since either way the arguments will have to accept both single strings or colletcions of strings.

the property already has a docstring, so you can remove this documentation of the attribute

see above

THis is the same logic that's used on __init__. Could you extract it to a new protected method (e.g. _parse_commands()) that you call both here and on __init__? Then you can also move the regexing from __init__ to that function. THat will make sure that also the setter only accepts valid bot commands (please add a unit test for this)

Sure, I first tried to do it like this
self.commands = command
But it wouldn't work when PrefixHandler tried to call it because it has a different setter than CommandHandler with the same name. But I'll make a function for it.

I noticed we don't have CommandHandler command limitations for PrefixHandler, so illegal commands in prefixhandler should work properly shouldn't they?

I noticed we don't have CommandHandler command limitations for PrefixHandler, so illegal commands in prefixhandler should work properly shouldn't they?

yes, since PrefixHandler is not limited to the commands recognized by Telegram. Actually the fact that PrefixHandler is a subclass of CommandHandler is more or less an implementation detail …

please add a "versionchanged" directive saying that it now also accepts sets

Please move the "changed to" notes to a versionchanged directive right before the Args:. Please move the "now accepts sets" to a versionchanged directive in the documentation of the respective arguments

Is this override still necessary? IISC, this does exatly the same as CommandHandler.commands

yes, because this class uses a different setter than the parent one and needs a property to set it.

IIRC using @CommandHandler.commands.setter instead of @commands.setter should do the trick and then you don't need to re-implement the property

please remove

The attribute command was replaced by :attr:commands

please revert - this is the reason the pre-commit test is failing

when this is raised, self._commands has already been changed - it should be the other way around. You can even add a unit test along the lines

1. bulid commandhandler with valid commands
2. try to assign invalid commands
3. assert that the commands where not changed

what we can do to make this easier here & also make the init a bit cleaner is:

1. make _parse_commands return the parsed commands instead of assigning them directly to self._commands. It can be converted into a static method
2. apply the regexing for valid commands directly within _parse_commands
3. do self._commands = self._parse_commands(command) both here and in __init__.

IISC this doesn't rebuilt self._combinations, but that's needed. I guess it should be self._build_commands instead. If you want to reduce code duplication some more, you can add a method _slts_to_set that returns {arg.lower()} if isinstance(arg, str) else {item.lower() for item in arg}. This can be a static method of CommandHandler or even just a module level function. You can then use it in CH._parse_commands, in the setter of prefixes and here.