Skip to content

Conversation

@MrZhang365
Copy link

The faster, the better.

@marzavec marzavec added the enhancement New feature or request label Sep 8, 2022
@marzavec
Copy link
Member

marzavec commented Sep 8, 2022

Two issues need to be addressed before this can be merged.

The first is a branch issue. It is a common practice to have a stable, production ready 'master' branch that is ready to be deployed by another person- as well as a branch for future changes and experimental / beta features. For more information, I would suggest reading an article like this (if you are able to access it, of course). You are attempting to commit to this master branch. This creates two problems. If this were merged, your changes will eventually be lost when the version-2-dev branch is merged into master (becomes a stable release). The other problem is that your changes cannot be deployed to the production server, as that server is running a version of the v2 dev code.

Faster operation
The faster, the better.

The second issue is, ironically, an issue with performance / speed decrease that your code incurs. It's a little complicated, so let me layout the reason that these hooks have been intentionally left out.

When a hook is added to a module, that hook function (branch) must fully execute and return. Currently there are twelve hooks into the chat module. Your commit adds another fourteen, for a total of twenty-six. As this is 'chat' software, the chat module is one of the most commonly invoked commands. Since admin and mod functions are so rarely invoked, this makes twenty-six (likely) useless function calls, every single time someone sends a message. In the time that it took to write this reply (maybe five or ten minutes), the production server had the chat module invoked about 3,000 times. Your commit would raise the function calls from 36,000 to 78,000 useless function calls.

For how rarely these modules are invoked, the performance impact is not worth it. Especially considering how easily the admin/mod commands can invoked otherwise.

Many chat rooms derived from hackchat can do this
such as CrosstChat(Henrize Chat), XChat, ZhangChat

This is not a matter of ability; of course the software can do this and handle it very well. That is the core idea of this software- to be a hackable communication solution for small communities (like the forks that you mentioned), to large communities. Small communities will be less concerned about performance and can choose to enable such things.

An argument may exist to allow a partial merge, but I'm interested in your reply.

@MrZhang365
Copy link
Author

!!!This is really a shocking number!!!
I never thought of calling so many useless functions this time!
Thank you for your support. I will use other methods to achieve this function. Let's wait and see tomorrow's pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants