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.

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

Code improvements & fix response packet size #22

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

WarningImHack3r
Copy link

  • Get rid of dependencies, cherry-picking modifications from mknjc's fork
  • Updating to Java 20
  • Update most dependencies
  • Some code improvements
  • Fix incorrect RconResponse response packet size
  • Move the example and add one
  • Update README and fix typo

Note
If you ever merge this PR, don't forget to change back the JitPack's badge in the README alongside pushing a new tag

- Get rid of dependencies, cherry-picking modifications from mknjc/minecraft-rcon
- Updating to Java 20
- Update dependencies
- Some code improvements
- Fix incorrect RconResponse response packet size
- Move the example and add one
- Update README and fix typo
Also print command on error message.
@MrGraversen
Copy link
Owner

Hi @WarningImHack3r, thank you for the contribution.

This is an old project and there are heaps of improvements I would like to make if time permitted 😁

I'm wondering, however, why you suggest to de-Lombok the project, instead of improving the usage of Lombok (there are many opportunities to do this)?

@WarningImHack3r
Copy link
Author

Hi @MrGraversen! I'm not really the author of that change, it originally comes from https://github.com/mknjc/minecraft-rcon, which I found to be the only fork with (interesting) changes.
I personally have nothing against Lombok, but it's true that it didn't seem really useful because a lot of things are doable natively in the latest versions of Java and the others that are not can easily be replaced.
So a dependency removed feels like a free win, doesn't it?

Side note: the last commit has been made for a feature I needed and is not related at all to this PR, but as it was on the main branch the PR automatically picked it, I can try to cherry-pick commits to a dedicated branch and remake that PR if you want

@WarningImHack3r
Copy link
Author

WarningImHack3r commented Aug 5, 2023

@MrGraversen I'm currently adding all missing commands, if you want to review the PR with my branch's current state you can do it now, because else everything will be included in the same PR. I plan to push the commit by today or tomorrow. If you don't mind then it's okay.

Side notes

  • I don't know yet precisely how to do it, but I plan to push a 2.0 with a big breaking change really soon: pass the game version to the main MinecraftRconService constructor and annotate enum values or methods which are not available in the game version you target. It'll be very useful for 100% safe usage.
  • I'll make sure to keep this project up to date in the future as it's closely linked to the project I'm working on, Mintstone. Be ready to see PRs from me regularly! Don't worry though, I use my fork as the main dependency so I don't directly rely on your approvals and your time, but I'll for sure port all my changes here too :)

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.

2 participants