-
-
Notifications
You must be signed in to change notification settings - Fork 32
Improved documentation for BlockState snapshots and indicated nullability #29
base: master
Are you sure you want to change the base?
Improved documentation for BlockState snapshots and indicated nullability #29
Conversation
…nto docs/blockstatesnapshots
why was this closed? |
Alright, I'm gonna reopen it then. No rush with the review though, I'll just reopen it and will be available once these changes are to be discussed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me. Some nitpicks on javadocs, but those should be relatively easy to just commit with GH's UI and we'll squash on merge.
Thanks for your contribution!
src/main/java/io/papermc/lib/features/blockstatesnapshot/BlockStateSnapshotHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/io/papermc/lib/features/blockstatesnapshot/BlockStateSnapshotHandler.java
Outdated
Show resolved
Hide resolved
...ain/java/io/papermc/lib/features/blockstatesnapshot/BlockStateSnapshotOptionalSnapshots.java
Outdated
Show resolved
Hide resolved
src/main/java/io/papermc/lib/features/blockstatesnapshot/BlockStateSnapshotResult.java
Outdated
Show resolved
Hide resolved
src/main/java/io/papermc/lib/features/blockstatesnapshot/BlockStateSnapshotResult.java
Outdated
Show resolved
Hide resolved
src/main/java/io/papermc/lib/features/blockstatesnapshot/BlockStateSnapshotResult.java
Outdated
Show resolved
Hide resolved
javaddocs and nullability is fine, but eh on the rename. That's an API break that I don't think is big enough value to warrant. Can just put a TODO to rename it if we ever justify a 2.0, but i'm not a fan of releasing a 2.0 just for that. also the functional interface doesn't really add any value. doesn't do anything nor does the intent of the handler even declare it SHOULD be a single method. don't see much value in adding that to only remove it later if we were to add another method (though extremely unlikely) |
Co-authored-by: Mariell <proximyst@proximyst.com>
I can understand this, I did expect the renaming to pose a rather unwanted change too, but I did include it at first to hear your opinions about the naming of these classes nonetheless.
I cannot really think of any additional methods for the interface, so I thought this would be a sensible addition. Thanks for the feedback! |
I really like PaperLib and it has helped us optimize our projects a lot so far. However I think some parts lack a bit of documentation, so I decided to propose a few changes.
1.
BlockStateSnapshot
I personally find this interface name a bit confusing, as it does not represent a snapshot but rather functions as a way to obtain block states, snapshot or not.
I have renamed this to
BlockStateSnapshotHandler
for now but I am open for other suggestions.I am also aware that this naming pattern is applied elsewhere, so let me know if this should remain unchanged.
annotations
I added a
FunctionalInterface
annotation to this class to indicate this as a single-method-interface.I don't think there will be any other methods added to this in the future but feel free to correct me on this.
I have also added
Nonnull
annotations to the method and all methods in it's implementations to indicate the nullability of this method and its arguments. This is also consistent with the rest of PaperLib, the annotations I used were from FindBugs which was also consistent with the rest of the project.documentation
I added a javadocs block to the interface, as well as the method inside which explains how this fits into the bigger puzzle. This can improve maintainability and help other developers understand what is going on.
The same applies to all implementations of this interface.
2.
BlockStateSnapshotResult
This class has received a bunch of documentation and
Nonnull
annotations.Anyway, let me know what you think of these changes and whether I got something wrong.