Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clean up AbstractPacket #57

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Clean up AbstractPacket #57

wants to merge 4 commits into from

Conversation

Photon-GitHub
Copy link

@Photon-GitHub Photon-GitHub commented Sep 27, 2018 •
edited
Loading

The "recievePacket" method has been deprecated for almost a year now and the replacement method is directly below the misspelled one. All plugins should have updated by now.

Instead of throwing the exceptions manually the Preconditions class is helping out.
Please note that I throw a NullPointerException if the packet handle is null and no longer an IllegalArgumentException. If this is not desired the line can be changed to
Preconditions.checkArgument(handle != null,"Packet handle cannot be NULL.");

@Photon-GitHub Photon-GitHub changed the title Remove old deprecated "recievePacket" method. Clean up AbstractPacket Sep 27, 2018
@Photon-GitHub
Copy link
Author

Also, @dmulloy2 should I make another pr for removing the other deprecated methods? I don’t know how long you want to keep deprecated code before removal.

yannicklamprecht
yannicklamprecht suggested changes Oct 11, 2018
View reviewed changes
Copy link

@yannicklamprecht yannicklamprecht left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Are the two spaces indentation instead of the 4 are desired by the repository owner?

@dmulloy2
Copy link
Owner

The thing about PacketWrapper is that it's meant to be be directly copied into plugins, so we're good to remove deprecated methods after the next Minecraft release. So anything that's currently deprecated is fine to be removed assuming people didn't just ignore the warning

@Photon-GitHub
Copy link
Author

Photon-GitHub commented Nov 12, 2018 •
edited
Loading

@dmulloy2 Anything that I am supposed to change here (intendation?) so that you can merge this pr?
I will make another pr for the other classes with deprecated code.

yannicklamprecht
yannicklamprecht suggested changes Feb 25, 2019
View reviewed changes
Copy link

@yannicklamprecht yannicklamprecht left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Remove the uneeded formating changes see single line comments.

yannicklamprecht
yannicklamprecht suggested changes Feb 25, 2019
View reviewed changes
Copy link

@yannicklamprecht yannicklamprecht left a comment

Choose a reason for hiding this comment

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

Uneeded formatting changes. Use 4 spaces instead of 2.

PacketWrapper/src/main/java/com/comphenix/packetwrapper/AbstractPacket.java

this.handle = handle;
}
/**
Copy link

Choose a reason for hiding this comment

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

changes from here ->

PacketWrapper/src/main/java/com/comphenix/packetwrapper/AbstractPacket.java
ProtocolLibrary.getProtocolManager().broadcastServerPacket(getHandle());
}

/**
Copy link

Choose a reason for hiding this comment

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

to here.

PacketWrapper/src/main/java/com/comphenix/packetwrapper/AbstractPacket.java
}
}

/**
Copy link

Choose a reason for hiding this comment

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

from here

PacketWrapper/src/main/java/com/comphenix/packetwrapper/AbstractPacket.java
} catch (Exception e) {
throw new RuntimeException("Cannot receive packet.", e);
}
}
Copy link

Choose a reason for hiding this comment

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

to here

JarvisCraft
JarvisCraft reviewed Sep 21, 2020
View reviewed changes
PacketWrapper/src/main/java/com/comphenix/packetwrapper/AbstractPacket.java
@@ -2,24 +2,25 @@
* PacketWrapper - ProtocolLib wrappers for Minecraft packets
* Copyright (C) dmulloy2 <http://dmulloy2.net>
* Copyright (C) Kristian S. Strangeland
*
* <p>
Copy link

Choose a reason for hiding this comment

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

This comment is not treated as a Javadoc so instead of adding <p> tags I would suggest removing the second * from the comment start

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JarvisCraft JarvisCraft JarvisCraft left review comments

@supertassu supertassu supertassu requested changes

@yannicklamprecht yannicklamprecht yannicklamprecht requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Photon-GitHub @dmulloy2 @yannicklamprecht @JarvisCraft @supertassu