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

Cleanup Commands #3935

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

Conversation

JustAHuman-xD
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD commented Aug 5, 2023

Description

This PR is made to cleanup all of the command classes & make them more readable, after this PR a PR could be opened to abstract a lot of these checks to further improve the state of the command classes, I am leaving that separate to avoid going out of scope and make it more manageable. Blame Jeff for the opening of this PR 👍

Proposed changes

Change almost all Command classes to use more guard statements, add missing annotations, and just general cleanup. This PR does go against the code style guidelines, discuss in developers chat

Related Issues (if applicable)

N/A

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
  • I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.20.*).
  • I followed the existing code standards and didn't mess up the formatting.
  • I did my best to add documentation to any public classes or methods I added.
  • I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
  • I added sufficient Unit Tests to cover my code.

@JustAHuman-xD JustAHuman-xD requested a review from a team as a code owner August 5, 2023 22:36
@JustAHuman-xD JustAHuman-xD added the 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. label Aug 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2023

Pro Tip!
You can help us label your Pull Requests by using the following branch naming convention next time you create a pull request. ❤️

Branch naming convention Label
feature/** 🎈 Feature
fix/** ✨ Fix
chore/** 🧹 Chores
api/** 🔧 API
performance/** 💡 Performance Optimization
compatibility/** 🤝 Compatibility

If your changes do not fall into any of these categories, don't worry. You can just ignore this message in that case! 👀

@JustAHuman-xD JustAHuman-xD added the 🧹 Chores Refactoring / Cleanup. label Aug 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2023

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: 0c9c8d8b

https://preview-builds.walshy.dev/download/Slimefun/3935/0c9c8d8b

Note: This is not a supported build and is only here for the purposes of testing.
Do not run this on a live server and do not report bugs anywhere but this PR!

Copy link
Contributor

@iTwins iTwins left a comment

Choose a reason for hiding this comment

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

Saw you missed these. Also SlimefunTabCompleter.java had some annotations that I couldn't comment on.
I think this looks a lot cleaner than it did before.

@JustAHuman-xD
Copy link
Contributor Author

I completely forgot about this whoops, I'll actually work on this today

@JustAHuman-xD
Copy link
Contributor Author

(Could I get some reviews and testing on this)

@WalshyDev
Copy link
Member

WalshyDev commented Dec 31, 2023

Needs rebase

https://pastebin.com/S1mUPrhp

@J3fftw1 J3fftw1 added ⚠BLOCKED This pull request is currently blocked from merging for technical reasons. Build tested Used to indicate the PR preview build has been tested by one of the team and removed ⚠BLOCKED This pull request is currently blocked from merging for technical reasons. 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. labels Dec 31, 2023
@J3fftw1
Copy link
Contributor

J3fftw1 commented Dec 31, 2023

I tested this on a version that doesnt break heads.
Since we need a rebase for that see walshys comment.
This is good to go
Tested all commands.

J3fftw1
J3fftw1 previously approved these changes Dec 31, 2023
@J3fftw1
Copy link
Contributor

J3fftw1 commented Jan 15, 2024

Can we get a rebase here?

@TheBusyBot TheBusyBot added the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Feb 10, 2024
@TheBusyBot TheBusyBot removed the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Feb 14, 2024
@TheBusyBot TheBusyBot added the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Mar 22, 2024
@TheBusyBot TheBusyBot removed the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build tested Used to indicate the PR preview build has been tested by one of the team 🧹 Chores Refactoring / Cleanup.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants