objects.py at_say method update adds flexibility, maintains backward compatiblility.

[BlauFeuer]

Brief overview of PR changes/additions

Returns use of the the "type" kwarg to the tuple msg output, allowing messages of different types, but having at_say default to "say" type, as before.

Defaults to using msg_self ("You say, ) format, but allows not excluding the speaker from msg_location messages. Keeps the hard-coded defaults, including whisper, but whisper mode is engaged with a kwarg or msg_type for flexibility/ compatibility.

Motivation for adding to Evennia

Game code relying on Evennia 0.6 behavior of sending type kwarg in tuple msg output won't break, but keeps the nice mappings and default says/whisper (albeit in a slightly modified format) that @v-legoff added.

Other info (issues closed, discussion etc)

devel branch

Does this change break anything differently, and will it fit the expected and more uncommon use cases?

[BlauFeuer]

This does break the unit test due to the whisper default format change.

evennia/commands/default/tests.py", line 136, in test_whisper
self.call(general.CmdWhisper(), "Obj = Testing", "You whisper to Obj, "Testing"", caller=self.char2)

Does anyone prefer one format over the other?

[BlauFeuer]

@chainsol Thanks for the feedback.

chainsol@#evennia: Format-wise, I prefer 'You whisper "Hey, buddy." to Someone', and 'chainsol whispers: "Hey, buddy."'

Based on my own preferences and the above comment, I change the whisper format default and unit test accordingly.

It's worth noting that msg_self is no longer optional, but setting it True uses the default msg_self string for say or whisper, and setting it None skips sending a different message to the caller and doesn't exclude the caller from seeing msg_location. Some will prefer msg_self to be None and some will prefer True, but out of the box, Evennia will use True with the default say and whisper.

If the docstring seems confusing, please comment regarding how it can be clarified -- otherwise, this PR should be ready to go into devel.

[TehomCD]

I find msg_self to being a different value and use than msg_location while having very similar docstrings to be a bit confusing - having never looked at the code before, it seemed like the documentation didn't match what the method now does. I think I'd find it more intuitive to do something like caller.at_say(speech, receiver=receiver, msg_self=caller_version_speech, msg_location=room_version_speech) if they varied. Removing the ability to say one thing to receivers and get a different value would make it harder to do things that might change the speech for a receiver and speaker (drunken-code where receivers see text as slurred but the sender does not, language-code where you create gobblygook or a <You don't understand that language>, etc).

It probably would make a little more sense to me if msg_type was just substituted in the different phrases as a verb by default, so it'd be say/says, whisper/whispers, shout/shouts, whatever.

Something else that might be worth thinking about is if receivers was an iterable in order to support sending one message to selected receivers in a room, and a separate message to anyone else in the room who did not quality. For example, if you wished to support 'whispering' to more than one person at a time, and then just sending 'Blau whispers quietly to Griatch, Chainsol' or whatever to the room.

I sort of feel that base hooks should try to capture as many use-cases as possible without having overwhelming complexity, but I don't think a list of receivers versus one would be more difficult to understand for a user implementing the hook.

[BlauFeuer]

I think the intent @vlegoff had with msg_self and msg_location was to be able to apply filtering to msg_location independently, but doesn't handle the use case of each listener in the location to receive a different msg_location, nor does CmdWhisper handle multiple coma-delimited targets.

I'm confused while trying to interpret what is meant by:

Removing the ability to say one thing to receivers and get a different value would make it harder to do things that might change the speech for a receiver and speaker...

since as far as I know, I didn't remove that, I just made msg_self optional, because I don't use it in my say (in the say way pose doesn't have a pose_self).

But, If msg_location and receivers did handle iterables of strings and objects, (an optional dict contained receiver keys with string values for each unique msg_location), then such a use case wouldn't have to call at_say multiple times for a list of receivers receiving custom messages. The same would be true of a custom CmdWhisper.

[Griatch]

I would say that the new msg_type kwarg to at_say should not be added - this information you can gleam from the (lack of) the already existing whisper kwarg, so adding this is superfluous. The text tuple with e.g. {"type": "say"} is intended for the client/prototocol (although we don't actually use it there yet, hence why it was removed). To parse it and use it on this side of the AMP is a hack - and now when these hooks supports **kwargs its like should not be necessary. But on the other hand we will need to send that data to the client eventually so if it is helpful to someone I'm fine with doing so again.

[BlauFeuer]

@Griatch The reason I restored msg_type was because it was in 0.6, in say, pose, and whisper and is still in 0.7's pose. It broke @CloudKeeper1 's implementation of the puppetbot when @vlegoff removed it, maybe just an oversight, since he could have used the type embedded in whispers as a marker for whispered text versus speech.

To have other objects parse it server side is the best we can do, as the whisper kwarg isn't included in the actual message sent whereas in this implementation similar to how it is done in 0.6, it's embedded in the tuple. When an object receives a message from another object without the type embedded in the tuple, the most the receiving object knows is which object sent it.

If default say, whisper, pose removes type from the tuple in 0.7, default say, pose, and whisper can no longer be used the way it was in 0.6, unless the commands injected the tuple to the hooks. For example CmdSay called at_say like caller.at_say((speech, {'type': 'say'}), msg_self=True), then at_say's speech (message (str): The text to be conveyed by self.) would have to accept str or tuple to get the same functionality in 0.6.

If you decide that at_say shouldn't accept msg_type, then I can rework this PR to send the tuple message with 'type': 'say' embedded in the message arg of at_say to maintain the same functionality.

[BlauFeuer]

Another feature of this PR is make excluding the caller from being sent msg_location optional, instead of mandatory; a mandatory exclusion of msg_location does not fit everyone's use_case for a say command, which is the first reason I had to rewrite CmdSay for my implementation. In 0.7, I would have to bypass using at_say entirely for my version of CmdSay to work.

Put plainly, without this, Evennia's at_say would enforce a specific (but perhaps marginally more popular) use case, and require being bypassed for the alternate use case where everyone (including the caller) sees '{object} says, "{speech}"' in the same way pose does.

[Griatch]

@BlauFeuer I'm not sure where 0.6's CmdSay would call at_say as caller.at_say((speech, {'type': 'say'}), msg_self=True). Do you mean location.msg_contents() maybe? This is otherwise CmdSay in 0.6: https://github.com/evennia/evennia/blob/master/evennia/commands/default/general.py#L387. CmdWhisper doesn't call at_say at all in 0.6. The {type:say} is passed only with the outputfunc to location.msg_contents. Am I missing something or are you referring to custom modifications in your own codebase?

I added **kwargs support to the at_say hook (and many other hooks) in 0.7. In 0.6 it only accepts caller and speech as arguments. At any rate - now that the hooks accepts arbitrary kwargs, it makes sense that whisper/say/whatever info should be passed by the relevant Commands making use of that hook.

[BlauFeuer]

@Griatch **kwargs support to the at_say hook isn't relevant to the issue this PR addresses, which is two-fold:

1. The loss of {'type': 'say'} embedded in the message that CmdSay issues.
2. The lack of support in CmdSay and at_say to make excluding the caller optional.

[Griatch]

Ok, just so we ware on the same page: 0.6 never passed the type to at_say, nor did CmdWhisper use the at_say hook at all. This is already something new for 0.7. That said, I think this summarizes my views:

• I'm okay with re-adding the type kwarg to the text outputfuncs going into msg_contents, this was removed but is something that the client will need eventually anyway. I still don't understand why you need this for parsing on the Server side though, is that for some custom overrides in the output flow (I vaguely remember talk about this)?
• The **kwargs addition is definitely relevant. The whole point of **kwargs is that you can pass whatever flags you want to your custom at_say from your custom say-like commands without conflicting with the official API.
• I don't think passing a bool to avoid the self-message is a good idea, it's confusing compared to how other similar-named args work. I suggest it's more intuitive to just look for the empty string and interpret that as if no self-message should be sent.
• msg_type is not needed as an explicit. In the default setup, looking for the whisper in **kwargs should be enough.
• Other commands using at_say should relay correct info to hooks and msg if they don't already.

[BlauFeuer]

@Griatch Here's all my thoughts I can express regarding the feedback I've received:

**kwargs passing

As my understanding goes, relevant to the discussion about msg_type, in that you mentioned including both would be "superfluous". Another point of view is that it's "backward compatible" and "flexible" to allow one or the other or both. The whisper=True kwarg is not part of the official API, but the PR does not remove support for whisper=True.

msg_type arg

The question remaining is what extend you want tuple messages to be part of the official API. Having messages sent via msg method by a string or by tuple with the first element being a string has legitimate uses, and you seem to support that. What's remaining is the use of {'type': } in messages. That usage need not be an official part of the API, but it is used in 0.6 in CmdSay, CmdWhisper, and CmdPose as what I imagine to be an example usage, one that @CloudKeeper1 leveraged so that the tuple in the message denoted the type of message being conveyed by the string portion of the message. So I'm assuming it's not "official", but it's a demonstrated usage in Evennia's default commands.

What I read from you as relevant to the PR is:

1. You don't mind at_say via CmdSay and CmdWhisper to send its message out as a tuple including the {'type':}; you say that can be done implicitly, but you don't want msg_type as an explicit arg to at_say. That's fine - CmdSay can pass a tuple, and at_say can pass it along to the msg method via msg_contents - I think it only requires a docstring change in at_say to mention that the message arg can take a "string or tuple"

2. For msg_self, you'd prefer an empty string (which like None and False still evaluates to False) be mentioned in the docstring, instead of mentioning a bool, though calling with a bool would still work implictly. The issue there is when wanting to use the say/whisper format defaults, an empty string denotes no msg_self and allows caller to receive msg_location, but what string value of msg_self would indicate that the default format strings for msg_self and msg_location are to be used?

How to update the PR to make it more acceptable:
If I understand your response to the PR correctly, it can be modified to conform. The only unanswered design question is part 2 above, how to indicate at_say should use default format for msg_location and msg_self without a bool being accepted for msg_self. Perhaps a kwarg for using msg_self's defaults being True, while an empty string for msg_self indicating False if no kwarg for self messaging is found. That's my take-away on the issues with the current implementation.

I could even address the parts I understand from @TehomCD and include accepting a dict for message and msg_location alternate formats per targeted object so that at_say allows for a single call to send a variety of formats and outputs after being processed through a language check to obscure parts of the message to some of the audience -- but that's somewhat beyond the initial scope of the PR.

[TehomCD]

I probably didn't explain myself clearly - I was confused by msg_self being changed from an optional argument, which it still states it is in the docstring, to a mandatory one, due to removing its default argument of None.

The use case I was addressing wasn't suggesting a dict for message/msg_location formats, but for having an iterable for the receivers where they would all be sent the specified speech, while msg_location was then used for the location which would receive a different message, probably with the receivers passed as the exclude argument. To me, the common use case for pose/say/whisper/etc has three separate messages I've had to handle in my implementations: a message to the sender, a message to the receivers, and a message to the room. It seems like your implementation of at_say almost handles this by default, but doesn't quite do it. My suggestion would be changing the default exclude to receivers for msg_location and making receivers an iterable and sending the message to each. That's just my opinion, though!

[Griatch]

@BlauFeuer Sure, this PR can certainly be made something that can be merged! We are almost there in the discussion I think. Still some confusion though I feel. :)

As my understanding goes, relevant to the discussion about msg_type, in that you mentioned including both would be "superfluous". Another point of view is that it's "backward compatible" and "flexible" to allow one or the other or both. The whisper=True kwarg is not part of the official API, but the PR does not remove support for whisper=True.

From an API standpoint there is no "backwards compatibility" to consider here since 0.6's (master branch) at_say supported none of this. We don't worry about backwards compatibility within devel branch, that defeats the point of a development branch. :) at_say on master only takes the speaker and the speech, that's it - and that's all it gets from CmdSay. 0.6 does however also pass the outputfunc tuple to location.msg_contents - this I agree should be put back in 0.7 as suggested.

You don't mind at_say via CmdSay and CmdWhisper to send its message out as a tuple including the {'type':}; you say that can be done implicitly, but you don't want msg_type as an explicit arg to at_say. That's fine - CmdSay can pass a tuple, and at_say can pass it along to the msg method via msg_contents - I think it only requires a docstring change in at_say to mention that the message arg can take a "string or tuple"

Just to be clear on what we refer to here. I refer to the outputfunc. That is, the call location.msg_contents(text=(speech, {"type": "say"}) at the end of CmdSay (and similarly at the end of CmdWhisper). The text outputfunc is relayed unmodified to to msg and in turn across the AMP to the relevant protocol, where the type could be used by the Client for custom output. I don't really see any reason to pass the outputfunc tuple to at_say though - not only does this complicate at_say, it's not the job of at_say to process this in the first place - it's intended as a string-manipulation hook. Pass the speech to it (along with 'whisper' or not) get then potentially modified speech back and then build the outputfunc tuple and pass it to msg_contents.

For msg_self, you'd prefer an empty string (which like None and False still evaluates to False) be mentioned in the docstring, instead of mentioning a bool, though calling with a bool would still work implictly. The issue there is when wanting to use the say/whisper format defaults, an empty string denotes no msg_self and allows caller to receive msg_location, but what string value of msg_self would indicate that the default format strings for msg_self and msg_location are to be used?

No, the argument would only accept strings or None (since it's optional). It would look for speech == "" explicitly to skip the self-message. None (unset) is the other case, which means using the default.

@TehomCD

My suggestion would be changing the default exclude to receivers for msg_location and making receivers an iterable and sending the message to each.

Hm, this is a little outside the scope of this PR, but I guess it would be useful to expand receiver (receivers is likely the better name then) to also be an (optional) iterable of objects excluded from the msg_location message.

[Griatch]

Since this is not a blocker for devel merge, I'm removing the devel tag from it

[BlauFeuer]

I ran out of time to work on this. Since there's a push to merge devel branch, I'm considering if breaking this into separate issues is a more workable solution.

I probably should have made an issue of it, but thinking devel would be in development longer and the issue would be addressed sooner, instead I only mentioned it in a comment of #1174 as something that breaks existing published game code.

[Griatch]

Ah, didn't intend to close this. Since this is essentially a bug fix not changing API, please re-open against the master branch.

[Griatch]

I have implemented most of this manually in latest master along with rewrites of the entire at_say hook. Should hopefully work the same way as intended here.