Skip to content
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

Hide unknow packet warning #4362

Merged
merged 5 commits into from
Apr 4, 2023
Merged

Hide unknow packet warning #4362

merged 5 commits into from
Apr 4, 2023

Conversation

omarcopires
Copy link
Contributor

Pull Request Prelude

Changes Proposed

Disable by default the Unknow Packet warning preventing spam in the console.

image

@EPuncker EPuncker requested a review from ranisalt April 2, 2023 03:15
@ranisalt
Copy link
Member

ranisalt commented Apr 2, 2023

You could create no-op packet handlers for these packet types instead:

local noop = PacketHandler(0x1C)
function noop.onReceive() end
noop.register()

@omarcopires
Copy link
Contributor Author

You could create no-op packet handlers for these packet types instead:

Thank you for suggesting this approach. While creating no-op packet handlers could indeed help to hide missing packets, it would only be a workaround rather than a proper solution, as it wouldn't actually add the missing packets to the codebase. Moreover, it could make the code harder to maintain and understand in the long term, as it would introduce unnecessary complexity and potentially obscure the actual packet flow.

Instead, I believe the best approach would be to hide the warning message only in production, and keep it visible in debugging mode, so that developers can identify missing packets and add them properly, as you mentioned. This would ensure a cleaner and more reliable codebase, while still allowing for the necessary flexibility to add new features.

That being said, I respect your opinion and appreciate your input. However, I won't be implementing this approach in this pull request, as I believe there are better ways to handle missing packets in the codebase.

@MillhioreBT
Copy link
Contributor

You could create no-op packet handlers for these packet types instead:

Thank you for suggesting this approach. While creating no-op packet handlers could indeed help to hide missing packets, it would only be a workaround rather than a proper solution, as it wouldn't actually add the missing packets to the codebase. Moreover, it could make the code harder to maintain and understand in the long term, as it would introduce unnecessary complexity and potentially obscure the actual packet flow.

Instead, I believe the best approach would be to hide the warning message only in production, and keep it visible in debugging mode, so that developers can identify missing packets and add them properly, as you mentioned. This would ensure a cleaner and more reliable codebase, while still allowing for the necessary flexibility to add new features.

That being said, I respect your opinion and appreciate your input. However, I won't be implementing this approach in this pull request, as I believe there are better ways to handle missing packets in the codebase.

I like your way of thinking, and by the way the suggestion that ranisalt told you, was missing the two points:

noop:register() -- instead noop.register()

@ranisalt
Copy link
Member

ranisalt commented Apr 3, 2023

@omarcopires my point is that it does not make sense to move packet handling to Lua, then create a flag in C++ to pass it to Lua to disable logging. Just comment out that line, I would approve it. We can uncomment it when debugging it necessary :)

@EPuncker EPuncker merged commit 22134bf into otland:master Apr 4, 2023
@omarcopires omarcopires deleted the hideUnknowPacketWarning branch April 5, 2023 05:31
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.

4 participants