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

Cleaned up Throwable try catches #3280

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

Conversation

RecursivePineapple
Copy link
Contributor

This removes a large number of Throwable try/catches. Several important try/catches were given a better error message.
This was tested in the latest nightly and it worked fine. I placed a few machines to make sure nothing important was broken, although many of the try/catches had no explanation. If they prevented a crash due to a bug, I have no way of verifying that the bug is still present.

@RecursivePineapple RecursivePineapple added the refactor For PRs rewritting a part of the code to have a nicer code overall. label Sep 25, 2024
@RecursivePineapple RecursivePineapple requested a review from a team September 25, 2024 17:55
@boubou19 boubou19 added the ongoing freeze - do not merge PR tagged with this do not meet the requirement to be merged during a freeze. label Sep 25, 2024
@Dream-Master Dream-Master removed the ongoing freeze - do not merge PR tagged with this do not meet the requirement to be merged during a freeze. label Dec 8, 2024
@serenibyss
Copy link
Member

@RecursivePineapple could you rebase this PR and address the comments if you're still interested in working on it?

@RecursivePineapple
Copy link
Contributor Author

@serenibyss sure, I'm working on something else right now but I can work on it later

@RecursivePineapple RecursivePineapple requested a review from a team December 19, 2024 21:37
Copy link
Member

@serenibyss serenibyss left a comment

Choose a reason for hiding this comment

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

Overall a nice cleanup, just a couple that I'm hesitant on

@@ -177,96 +176,92 @@ public void updateEntity() {
} else {
tTime = 0;
}
try {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not confident on removing this one, have you tested it specifically?

@@ -130,32 +128,26 @@ public int checkRecipe() {
this.mOutputBlocked += 1;
} else if ((GTUtility.isStackValid(aStack)) && (aStack.stackSize > 0)) {
if ((getFillableStack() != null) && (getFillableStack().containsFluid(Materials.Honey.getFluid(100L)))) {
try {
Copy link
Member

Choose a reason for hiding this comment

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

This one too

@Dream-Master
Copy link
Member

@RecursivePineapple any update here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor For PRs rewritting a part of the code to have a nicer code overall.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants