This isn't quite about a bug or the like. Think of it like a subissue of #3, but it's just a design discussion that have to do with the merge:tm:.
As of right now, molter
(essentially) analyzes parameters (for its own use) upon function decoration. This may seem sane, but there is one issue: dealing with self
parameters (or rather finding out when they're there and ignoring them if they are).
self
isn't exactly a special variable in Python - it can be named anything, and isn't typehinted or marked as anything special. We want to avoid it if it is there (it's always possible to declare a command in __main__
, and is intended behavior), but... well, we can't. Not really.
molter
does the next best thing and checks if the function is "nested" - if it's in something, classes included. Based on that, we can theoretically ignore self
if it is. However, this method uses a hack and is a hack, having many cases where this would fail. As such, another method is likely needed. Maybe.
There aren't any better ways until molter
is merged, but once it is...
Solution 1
Analyze the function as it is being added to the bot's dicts of prefix commands. As this is done with the context needed to determine if the command is in a scale (or in the bot class) or not, this is possible.
The best way to do this is likely to add an extra argument to add_message_command
that indicates if the underlying function has a self
statement or not (defaulting to it not), and then running the parameter analyzing in the function itself. This also allows people adding their own commands manually to have some sort of control over that process, too.
Basically:
def add_message_command(command: MessageCommand, *, has_self: bool = False):
command.parse_parameters(has_self=has_self)
... # rest of logic here
# scales and in-bot commands can simply make has_self True when running that
There is one drawback: if invalid parameters are passed (an invalid converter, etc.), then the user won't found out it's invalid until the bot is loaded. However, this method has the benefit of allowing all associate_converter
s to be associated before the command is parsed, meaning we don't have to dig into the parsed parameters to replace things as needed.
Solution 2
Analyze all parameters (minus whatever one's the first one) on function decoration. Then, if the command is being loaded in a place that indicates the function has self
, edit the parameters list so that the first element is removed.
This is very simple, in like with what dis-snek
does, and allows for errors to happen pretty early on. However, this means that associated converters have to do what they currently do and dig into the current parameters to adjust them as needed, which is somewhat ugly. This also makes globally_associate_converter
not be able to apply retroactively, and also makes it a pain to do if we do what Silasary mentioned here, as, well, we wouldn't get the bot's associated converters until after the parameters have been parsed and messed around (and so they would have to be messed around with *again).
Conclusion
I'm leaning towards solution 1 personally, but I'm curious to see what everyone else thinks. I also do wonder if it's even worth the pain when the hack works... fine in most cases.
Again, I'm looking for ideas and feedback, so do say your thoughts!
help wanted