Skip to content

Some major changes style wise...#81

Closed
jotuel wants to merge 5 commits intomainfrom
101-experimental-breaking-changes
Closed

Some major changes style wise...#81
jotuel wants to merge 5 commits intomainfrom
101-experimental-breaking-changes

Conversation

@jotuel
Copy link
Copy Markdown
Collaborator

@jotuel jotuel commented Sep 14, 2025

...but really nothing major changed logically. This is a draft and more of a RFC. More changes like this could still be done. For me it improves readability, someone might see it otherwise.

flaw fixed in InviteCommand, the else if ch is nullptr sends a response
and returns now. Also if you change nick it won't send you the message
multiple times.
operator parts. Also moved macros to separate header and added even more
of them.
@jotuel jotuel marked this pull request as ready for review September 15, 2025 14:44
@jotuel
Copy link
Copy Markdown
Collaborator Author

jotuel commented Sep 15, 2025

I'm starting to like this macro approach more and more. Moving stuff to Server constructor makes sense. And now we only create a persistent CommandDispatcher instance per Client, just like a User, which makes more sense from memory usage point.

Probably one instance would've been enough, this works too. Its way faster as there is only one initialization for connecting client instead of one per request.

Overall from my perspective it makes sense to merge all of these changes instead of fixing these again separately. If not there is one crucial fix here, I made a mistake that Channel was not removed from the User in the case that it became empty and was removed from Server, so that would need to be patched anyways.

@jotuel
Copy link
Copy Markdown
Collaborator Author

jotuel commented Sep 21, 2025

Closing as these changes are already incorporated into a newer pull request.

@jotuel jotuel closed this Sep 21, 2025
@oliynykmax oliynykmax deleted the 101-experimental-breaking-changes branch September 28, 2025 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant