feat: improve commands_extension

[TheOneWithTheBraid]

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

1. Currently, even Client-level commands such as /join, /dm etc. require to be entered with a Room present as parameter
2. 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.
3. 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

[coder-with-a-bushido]

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]

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]

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]

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]

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]

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 😊

[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

[TheOneWithTheBraid]

@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 !

[coder-with-a-bushido]

@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]

Tests should now be fixed, @coder-with-a-bushido !

[TheOneWithTheBraid]

@coder-with-a-bushido Can you elaborate on what's wrong with the coverage test ?

[coder-with-a-bushido]

@coder-with-a-bushido Can you elaborate on what's wrong with the coverage test ?

I don't understand it either. The test is breaking in two totally different files, let me try it locally first. Meanwhile, can you pls address the 2 other comments left?

[coder-with-a-bushido]

@coder-with-a-bushido Can you elaborate on what's wrong with the coverage test ?

I don't understand it either. The test is breaking in two totally different files, let me try it locally first. Meanwhile, can you pls address the 2 other comments left?

I think you should just rebase it to upstream/main. That should work.

Also, make sure to squash your commits and add "BREAKING" right after "feat: " in the commit message.

[coder-with-a-bushido]

@TheOneWithTheBraid there was a dependency issue that's fixed in main now. If you rebase to main, that should fix the e2ee tests. I'll have to check why the coverage fails properly.

All that's left would be to address the 2 left out comments, squash the commits and add a "BREAKING" in the commit message. I'm sorry this got stuck so long.

[TheOneWithTheBraid]

@coder-with-a-bushido I finally found the time to add the relevant documentation. Please ensure to add the BREAKING when squashing and re-signing the commits.

[coder-with-a-bushido]

LGTM :)

stale