feat: improve commands_extension #1937

TheOneWithTheBraid · 2024-10-12T15:46:34Z

While implementing some custom commands, I stumbled across some issues with the current command runner :

Currently, even Client-level commands such as /join, /dm etc. require to be entered with a Room present as parameter
Currently, commands have no way to provide output, such as a success message, some reference to e.g. a newly created room or any other information since the return parameter is currently usually an eventId used by the clients to confirm sending a message was successful.
Currently, [matrix] clients have no way to catch exceptions specific to the command execution, such as a missing or invalid parameter etc.

I have the following proposals to address these issues in the SDK's command execution :

unify behavior of all message sending related command
add a StringBuffer as stdout-like output buffer for commands
create a typedef for the command function signature
create a common exception type for command execution
enable commands to run on Client-level rather than Room-level
BREAKING: Client.addCommand signature now takes an optional StringBuffer as second parameter

lib/src/utils/commands_extension.dart

coder-with-a-bushido · 2024-10-14T06:40:28Z

This PR looks really good to me except that one thing I commented about :)
Thank you so much!

krille-chan

May I ask where the issue is for this refactoring? Such a big change should go through a refinement first IMO

TheOneWithTheBraid · 2024-10-16T09:58:14Z

May I ask where the issue is for this refactoring? Such a big change should go through a refinement first IMO

I'm not bound to any work processes when I'm contributing in my free time. The PR has half a page of documentation why and how the change was made. If you have question or would like to discuss the implementation, feel free to leave a constructive review comment. Since the API was not changed in a way relevant for client developers, I do not see the need of outlining a dedicated change proposal in advance.

krille-chan · 2024-10-17T06:16:40Z

May I ask where the issue is for this refactoring? Such a big change should go through a refinement first IMO

I'm not bound to any work processes when I'm contributing in my free time. The PR has half a page of documentation why and how the change was made. If you have question or would like to discuss the implementation, feel free to leave a constructive review comment. Since the API was not changed in a way relevant for client developers, I do not see the need of outlining a dedicated change proposal in advance.

But !WE! are bound to work processes! It would just have been nice to communicate such a change in advance instead of just expecting that we would spend our time in reviewing hundreds of code lines without knowing if it would have any benefit.

TheOneWithTheBraid · 2024-10-17T06:45:11Z

Yeah, I see the problem. It's quite hard to have work processes on an open source open contribution project. Maybe some outline on starting from what amount of change (e.g. everything which is a new feature, changes API etc.) one should write an issue for discussion ? Might also help other external contributors ?

Currently the CONTRIBUTING.md only sais to open a merge request and discuss over there, that's why I so far did that for personal contributions, maybe you can update this (btw. it still sais GitLab too ^^).

krille-chan · 2024-10-31T12:24:17Z

Yeah, I see the problem. It's quite hard to have work processes on an open source open contribution project. Maybe some outline on starting from what amount of change (e.g. everything which is a new feature, changes API etc.) one should write an issue for discussion ? Might also help other external contributors ?

Currently the CONTRIBUTING.md only sais to open a merge request and discuss over there, that's why I so far did that for personal contributions, maybe you can update this (btw. it still sais GitLab too ^^).

okay maybe I should rephrase my comments so far. Of course it is okay that everyone opens Pull Requests and we will always do our best to review them. The problem is only that we cannot guarantuee that the changes get merged. Especially for changes which change the API. Maybe we are not satisfied with how it looks like, the approach to fix it and so on.

The interest conflict here is mostly because external contributors have the desire to fix one specific thing, but usually don't have to deal with the code base after it, while we have to deal with the code base as it is our daily work. So we have a strong desire to keep the code base in a level of quality, which fits our needs.

For small changes this shouldn't be a big deal. For big changes, which apply to hundreds of code lines, it could mean, that we either request to change a lot of things so that it matches our interests as well, or we completely decline it at all. Then the whole work on the Pull Request would be nearly gone with the potential to leave the author back with some frustration.
Therefore we suggest to propose bigger changes in an issue first. Opening an issue, explaining the need and the idea how to solve it. Then we could comment if we think this is a good idea or give feedback before the implementation starts. This could drastically reduce the dangers that the Pull Request in the end gets merged.

In the end one could argue that it is not our problem but leaving pull request authors back with frustration might lead to shit talking about us. Especially from you I received a lot of FluffyChat pull requests where I felt a lot of pressure to merge them and even frustration when it took longer. This is something which could have been avoided.

I admit that my previous comment does not really reflected this and we should update our contribution guideline.

krille-chan · 2024-11-07T10:29:20Z

lib/src/room.dart

+    String msgtype = MessageTypes.Text,
+    String? threadRootEventId,
+    String? threadLastEventId,
+    StringBuffer? commandStdout,


Can we add a little bit dartdoc comments to make it more intuitive how to use the StringBuffer? Or at least a link to the documentation. For example I see this type for the first time and would have to look it up before using it.

We might want to have more information from the output as just a String, as errors could also be interesting, while we also might get errors and values at the same time. Could be probably solved by introducing a CommandResult type which contains the success data, (Matrix-)errors as well as the output of the command

Hmmmm, in my implementation I simply used the regular error catching around the Client.parseAndRunCommand() and Room.send* methods for error handling. Since there are decent, unified Error types available I don't see an advantage in a dedicated error collection mechanism ; but if you have any proposal on how to implement some improved error handling, I'm curious for suggestions !

The advantage of a StringBuffer is that it basically allows to perform random access (similar to a File.open call) and object-oriented append syntax. I simply considered this as way more convenient for this use case - since a String could easily accidentally lose it's reference and doesn't support such a beautiful syntax to access its onwardly growing contents during the asynchronous writing into it.

@TheOneWithTheBraid Can we please have dartdoc comment for the commandStdout that says it's used for the command output in case a command was executed

@TheOneWithTheBraid ping

@TheOneWithTheBraid Can we please have dartdoc comment for the commandStdout that says it's used for the command output in case a command was executed

lib/src/client.dart

krille-chan

We looked at it on the refinement meeting together and we agree with the technical approach and the API changes. The usual review will follow. Thank you very much for the contribution 😊

lib/src/utils/commands_extension.dart

coder-with-a-bushido · 2024-12-10T12:02:33Z

lib/src/utils/commands_extension.dart

@@ -91,8 +98,12 @@ extension CommandsClientExtension on Client {

  /// Register all default commands
  void registerDefaultCommands() {
-    addCommand('send', (CommandArgs args) async {
-      return await args.room.sendTextEvent(
+    addCommand('send', (args, stdout) async {


we could add the eventId in the stdout here?

I'd object this since sending an Event is the default "action" on the Room.send. The event ID always used to be returned as result of the method, not as command output. I'd rather see the standard output StrinBuffer as a complimentary concept for additional information which is exactly not the default behavior (aka sending an event).

Consider the following flow :

As a user, I enter something into a message text field of my matrix client. I send the contents via the Room.send* methods.

The matrix client underneath now does not know at first if I just sent a regular message or if a command was executed. As the matrix client, I will know about this by checking the stdout. If there is data in the stdout, I consider the result as command result and show e.g. a dialog or a snack bar with a details button where I can see the stdout.

If the stdout is empty and I simply got back an event id as return value, as a matrix client, I simply know the user just sent a message.

If we'd by default add the event id to the stdout, the entire flow of determination whether a command with special result was executed or just a simple event was sent stops working.

I'm just trying to understand the use case for stdout once again, pls correct me if I'm wrong.

I use Room.send with a text while I'm unsure if it's actually a command or just a text that needs to be sent. So, what I do is I create a stdout object that will return me something (can be any meaningful data) when it's a command that does something other than sending an event.

If that's the case, as the user of the SDK I want 2 things to be done:

Dartdoc comments that clearly explains this context wherever stdout is mentioned.

An example usage of this scenario in both README and dartdoc comment above Room.send.

If I didn't understand it correctly, please tell me.

lib/src/utils/commands_extension.dart

test/commands_test.dart

coder-with-a-bushido

@TheOneWithTheBraid Sorry for the super late review 😖

Code looks really good to me except for those nitpick comments. Now that you've used the stdout in some commands, I somehow want to use it all commands possible so it can be used

- unify behavior of all message sending related command - add a StringBuffer as stdout-like output buffer for commands - create a typedef for the command function signature - create a common exception type for command execution - enable commands to run on Client-level rather than Room-level - BREAKING: Client.addCommand signature now takes an optional StringBuffer as second parameter Signed-off-by: The one with the braid <[email protected]>

Signed-off-by: The one with the braid <[email protected]>

TheOneWithTheBraid · 2024-12-13T13:13:03Z

@coder-with-a-bushido Thanks for the feedback ! I (hopefully) fixed the test and added two comments above, I'd appreciate some feedback. Have a good weekend !

Signed-off-by: The one with the braid <[email protected]>

coder-with-a-bushido · 2024-12-17T12:06:01Z

@coder-with-a-bushido Thanks for the feedback ! I (hopefully) fixed the test and added two comments above, I'd appreciate some feedback. Have a good weekend !

Hey, thank you. But looks like 1 test is still broken and I think 2 more comments should be resolved before we can merge this :)

TheOneWithTheBraid requested a review from a team as a code owner October 12, 2024 15:46

coder-with-a-bushido reviewed Oct 14, 2024

View reviewed changes

lib/src/utils/commands_extension.dart Show resolved Hide resolved

krille-chan requested changes Oct 16, 2024

View reviewed changes

krille-chan reviewed Nov 7, 2024

View reviewed changes

lib/src/client.dart Show resolved Hide resolved

krille-chan reviewed Nov 7, 2024

View reviewed changes

TheOneWithTheBraid force-pushed the braid/command-runner branch from b0fab79 to 33ee140 Compare November 7, 2024 17:54

TheOneWithTheBraid force-pushed the braid/command-runner branch from 33ee140 to b323617 Compare December 7, 2024 09:32

coder-with-a-bushido reviewed Dec 10, 2024

View reviewed changes