Minestom
Minestom copied to clipboard
Change assert in NamespacedID.from() to throw IllegalArgumentException instead
Fixes #1097.
Java programs have assertions turned off by default, and therefore you can register invalid namespaces with Minestom if you do not have assertions on.
This PR fixes it by replacing the assertions with exceptions instead.
Re:
Java programs have assertions turned off by default it is the original reason why these were assertions
I also think that it depends a lot on the context, if we're witnessing a misunderstanding about Assertions or not (but I also don't know Minestom's Codebase enough): Assertions are off by default so that they show now runtime overhead, but assert semantic failures in development builds (e.g. similar to how other languages have Debug and Run configurations)
It now depends on the context of this failure on whether we should rather push Assertions and basically build some documentation, showing how to run the server with assertions enabled (and to use that on QA/Testing environments, in case you don't run the server from the IDE).
Since using an invalid namespace does not cause any major fuck-up in unrelated parts (??), one could argue that an assert is still the right thing. You'll also immediately get feedback by the client when using such a namespaced key illegally. But then, those IDs are probably cached, so the code path is not too hot and Minestom doesn't seem to use asserts very widely anyway.
Pick your Poison: Learning Curve of running with -ea or a runtime overhead in code bases that do it right.