Add entity removal reason API #13075
Conversation
| case UNLOADED_TO_CHUNK -> RemovalReason.UNLOADED_TO_CHUNK; | ||
| case UNLOADED_WITH_PLAYER -> RemovalReason.UNLOADED_WITH_PLAYER; | ||
| case CHANGED_DIMENSION -> RemovalReason.CHANGED_DIMENSION; | ||
| default -> null; |
There was a problem hiding this comment.
removal reason shouldn't blindly map to null, if there is a value we don't have mapped, it should be reported (and ideally prevented by having a test to validate that everything is mapped)
There was a problem hiding this comment.
Sure! Let me see how to make a test
There was a problem hiding this comment.
Does it really need a test? Switch expressions are exhaustive, so if reasons are added/removed, it will just fail to compile without a default case. Maybe one for testing the mapped names match? which, bleh
There was a problem hiding this comment.
Does it really need a test? Switch expressions are exhaustive, so if reasons are added/removed, it will just fail to compile without a default case. Maybe one for testing the mapped names match? which, bleh
reasonable, then I'll just remove the default case and add a null case
There was a problem hiding this comment.
I wasn't sure if the 'new' expressions changed something there, but, if that's the case then probs not a big deal, would be nice to have some form of validation to prevent desyncing of stuff, just a trivial thing which is easy to miss
There was a problem hiding this comment.
Can these values not be generated?
Yeah it's more reasonable to generate it, but I'm not quite sure how to write the generator so I have to learn it. This may take some times...
There was a problem hiding this comment.
Yea it should be generated if you don't plan to add javadoc for each values, see https://github.com/PaperMC/Paper/blob/main/paper-generator/src/main/java/io/papermc/generator/Rewriters.java#L119 for an example (Panda$Gene). Then the switch can be replaced by apiEnum.valueOf(internalEnum.name()) with the null check.
There was a problem hiding this comment.
Yea it should be generated if you don't plan to add javadoc for each values, see https://github.com/PaperMC/Paper/blob/main/paper-generator/src/main/java/io/papermc/generator/Rewriters.java#L119 for an example (Panda$Gene). Then the switch can be replaced by apiEnum.valueOf(internalEnum.name()) with the null check.
Thank you for your guidance! This is very helpful.
There was a problem hiding this comment.
Looks almost good, #argument was used for single argument on the builder but in your case your enum take multiple arguments so #arguments should be used there. Can also use an inlined return tag for the javadocs.
There was a problem hiding this comment.
also use an inlined return tag for the javadocs.
Your guidance was very helpful, thank you!
I've made changes as you said. Hope it is okay this time.
NoyException
force-pushed
the
removal
branch
2 times, most recently
from
c163aa2 to
67abf42
Compare
|
I've replaced that enum with the generated values, thank you all for your help. |
| */ | ||
| public void remove(); | ||
|
|
||
| // Paper Start |
There was a problem hiding this comment.
I don't think these comments are required anymore.
There was a problem hiding this comment.
idk, I just saw that it was written the same way in other places. Should I delete them?
There was a problem hiding this comment.
Yes. Only Minecraft code needs additional marking of changes made by paper.
There was a problem hiding this comment.
Understood. I deleted them just now.
NoyException
force-pushed
the
removal
branch
from
67abf42 to
cdbc90c
Compare
NoyException
force-pushed
the
removal
branch
from
cdbc90c to
41fe010
Compare
Sometimes we want to know why an entity is removed in an easier way instead of listening
EntityRemoveEvent. So here is the API.