-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: improve commands_extension #1937
base: main
Are you sure you want to change the base?
feat: improve commands_extension #1937
Conversation
- 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]>
/// a [CommandException] should be thrown if the specified arguments are considered invalid | ||
typedef CommandExecutionCallback = FutureOr<String?> Function( | ||
CommandArgs, | ||
StringBuffer? stdout, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the stdout
used in any of the default commands. Even though having an output buffer sounds like an important thing to have, I doubt how useful it'll be. I just can't think of a really good use-case for it right now, could you pls tell me a good way it can be used (to output useful data) with the default commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's true, I didn't change the behavior of the default commands. Mostly, since they do not really have anything to output because they are pretty static commands without any kind of user interaction.
I personally use the stdout to modify the message text field : I e.g. implemented a /poll
command assisting in sending composing Element polls, I added a /search
command returning a protobuf content stream. I intentionally didn't specify any high-level format to use for stdout since I can imagine infinite different use cases how clients would like to interact with the output of commands that is not an eventId - that's why ; a simple StreamBuffer
and nothing further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One could consider adding JSON output of the /dm
and /create
commands containing the room ID though. If you like, I will add this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coder-with-a-bushido See latest comment. I added a helper class for command output and now add the room IDs of created/searched rooms to the stdout in the default commands.
This PR looks really good to me except that one thing I commented about :) |
Signed-off-by: The one with the braid <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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 ^^). |
While implementing some custom commands, I stumbled across some issues with the current command runner :
/join
,/dm
etc. require to be entered with aRoom
present as parameterI have the following proposals to address these issues in the SDK's command execution :
StringBuffer
as stdout-like output buffer for commandstypedef
for the command function signatureClient.addCommand
signature now takes an optionalStringBuffer
as second parameter