Skip to content

Conversation

@ProgrammerDan
Copy link

for evaluation and fixes before merge

Copy link
Author

@ProgrammerDan ProgrammerDan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly leaving comments for myself.

Final bit is for config.yml -- new contributions don't have a default

plugin().log(yml.saveToString());
sender.sendMessage(yml.saveToString());
}
YamlConfiguration yml = new YamlConfiguration();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing a valid null check? huh?

} else {
sb.append(" (none)");
}
sb.append(String.format(" %16s", player.getName()));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, removing null checks that were added due to observed defects...

It's possible the spigot / bukkit has improved but generally these checks are defensive to prevent NPE propogation in the case that a spigot implementation is flawed, or in this case -- an event is fired and the underlying data model is out of sync, and partially "invalid" per contract. Best to keep them in...

sb.append(String.format(", %16s v %16s",
event.getDamager() != null ? event.getDamager().getName() : "--unknown--",
event.getEntity() != null ? event.getEntity().getName() : "--unknown--" ));
sb.append(String.format(", %16s v %16s", event.getDamager().getName(), event.getEntity().getName()));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More removed, worthwhile nullchecks.

Bukkit.getServer().getPluginManager().callEvent(pbee);
if (pbee.isCancelled()) {
return;
if (e.getBedEnterResult() == BedEnterResult.NOT_POSSIBLE_NOW || e.getBedEnterResult() == BedEnterResult.NOT_SAFE) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self to test, great news if they are firing the normal event now


public void adminCloseInventory(InventoryCloseEvent event) {
if (event.getPlayer() != null && this.adminsWithInv.contains(event.getPlayer().getUniqueId())) {
if (this.adminsWithInv.contains(event.getPlayer().getUniqueId())) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to leave this null check off, but I suspect it was there with a reason too, just nothing as specific coming to mind as with the others.

ProgrammerDan and others added 19 commits September 4, 2019 00:36
…t some were hard earned. I do hope that Spigot is more faithful and careful in these later releases, but better to check and be sure then not check and be unsure. Some I did not add back in. Any I added back in I wrote why I added it. I also switched around some orderings to use equals() checks, and also addressed some changes to map views in new releases, although there is more to do there. As well, expanded certain block lists and biome lists even further, lots of new content in these new minecraft releases. Probably first of several commits, as I test.
… a new dragon damage for servers that want to allow dragon spawning but would prefer they dont remove the world en mass. Untested at yet. Some other fixes as noted in my self-review.
Fixed issue where onPlayerInteract() would re-enable cancelled event and removed duplicated enderchest check
@Maxopoly Maxopoly deleted the civ14 branch May 2, 2020 01:57
Maxopoly added a commit to Maxopoly/SimpleAdminHacks that referenced this pull request Aug 4, 2020
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