PaperMC / PaperLib

Plugin Library for interfacing with Paper Specific API's with graceful fallback that maintains Spigot Compatibility, such as Async Chunk Loading.
MIT License
269 stars 31 forks source link

Improved documentation for BlockState snapshots and indicated nullability #29

Open TheBusyBiscuit opened 4 years ago

TheBusyBiscuit commented 4 years ago

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.

MiniDigger commented 3 years ago

why was this closed? I am sorry that it is taking so long to review this, but this looks like something that would provide value

TheBusyBiscuit commented 3 years ago

Alright, I'm gonna reopen it then. I thought that this went stale after three months and decided it is small enough to just close it.

No rush with the review though, I'll just reopen it and will be available once these changes are to be discussed.

aikar commented 3 years ago

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)

TheBusyBiscuit commented 3 years ago

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.

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 am gonna undo the name changes in a follow-up commit then.

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)

I cannot really think of any additional methods for the interface, so I thought this would be a sensible addition. But I can understand if this choice was left open by design, after all noone's probably gonna rely on it to be a Single-Method-Interface in their code anyway, so I am gonna remove that too then.

Thanks for the feedback!