[go: nahoru, domu]
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

1.19-1.20 successful load #121

Closed
wants to merge 10 commits into from
Closed

1.19-1.20 successful load #121

wants to merge 10 commits into from

Conversation

Bunnky
Copy link
@Bunnky Bunnky commented Jun 24, 2022 •
edited
Loading

Description

Attempted to update STB to successfully load in 1.19, no other goals.

Changes

Updated dependencies, added MC 1.19 to versions, and attempted to pass the plugin instance inside ProtectionManager

Related Issues

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • 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

@Bunnky Bunnky requested review from a team as code owners June 24, 2022 09:52
@Sefiraat
Copy link
Member

Nerd

(I'll check this out later today :) )

@Sefiraat
Copy link
Member
Sefiraat commented Jun 25, 2022 •
edited
Loading

Oki dokie!

So, we do not want to add Commons Lang (as discussed in discord, not wrong by any means, just not what we want here :) )

So, for all the current Validate statements, you'll want to replace them with Preconditions from google.common.base. In near all cases you'll want Preconditions.checkArguement with the boolean expression. For those that currently check nullability you'll still want checkArguement with a check for object != null or object == null which ever is appropriate as Precondition's null checks throw an NPE instead of an InvalidArguementException which is preferable here.

For the IntRange usages, you'll just want a new class that has min, max and contains as this is the only methods I can see used in the plugin, should be an easy class to make an substitute but I'm happy to handle that part if you prefer?

StringUtils is used a few times. repeat can be handled by upping the language level to 11 for String.repeat and I can't see a reason not to do this Ignore what I said here, use Strings.repeat from google.common.base.

For the two instances of isNumeric then a simple util method needs to be added for this. Either replicating the method currently being used or another one. Im tempted to do a test Double.parseDouble and return true/false based on the results. Happy to help here also.

That should clear all of the usages of commons.lang allowing us to not include the whole lib.

You know where I am so hit my on discord with any finite questions or is you want me to push anything to your branch to help out :)

Love that you've been willing to take this one.

Would be interested to hear @TheBusyBiscuit 's views also as he may have some better solutions to what I've put here/possibilities for items to add to dough?

@Sefiraat Sefiraat self-assigned this Jun 25, 2022
Bunnky
Bunnky commented Jun 27, 2022
View reviewed changes
Copy link
Author
@Bunnky Bunnky 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.

687deb2

First was removing the commons-lib, and replacing many of the org.apache.commons.lang with com.google.common.base. Mostly Validate -> Preconditions as suggested. I did not understand the key difference between Validate.isTrue, Validate.notNull, and Validate.noNullElements. In all cases, I used Preconditions.checkArgument just based off the suggestion. I did read some other similar functions like Preconditions.checkNotNull, but I tried to stick with as basic as possible.

I replaced StringUtils.isNumeric with STBUtil.isNumeric. It didn't throw errors, but with most things I'm not sure if this is proper.

6cb951a

This was the hardest part by far and I am sorry if I did something wrong. First thing, figuring out classes needing to be its own file through me through a loop, so I winged it and put them in io.github.thebusybiscuit.sensibletoolbox.utils. I hope this is correct.

Second, I named them the same as what I was replacing: IntRange and WordUtils (<-- WordUtils was a curveball). For both IntRange and WordUtils, I just tried to do what apache-commons did. No errors, so assume correct again.

For the WordUtils, I double checked after compiling that signs on BSU's and HSU's are proper, and seem to be:

BSU_SignTest

2ad4438

Here I just replaced the imports and fixed the some of the old statements mostly regarding IntRange. Theres a Validate.isTrue in there that was replaced with Precondistions.checkArgument as well. No errors, so I'm assuming something is correct!

Overall, the plugin loaded, and worked after making all these changes. I am sure something or everything is completely wrong, so I welcome my next set of fixes :D

Thank you so much @Sefiraat for guiding me through this process.

@Sefiraat Sefiraat self-requested a review June 27, 2022 10:40
Sefiraat
Sefiraat reviewed Jun 30, 2022
View reviewed changes
src/main/java/io/github/thebusybiscuit/sensibletoolbox/api/items/BaseSTBMachine.java Outdated Show resolved Hide resolved
src/main/java/io/github/thebusybiscuit/sensibletoolbox/utils/IntRange.java Outdated Show resolved Hide resolved
src/main/java/io/github/thebusybiscuit/sensibletoolbox/utils/IntRange.java Outdated Show resolved Hide resolved
src/main/java/io/github/thebusybiscuit/sensibletoolbox/utils/IntRange.java Outdated Show resolved Hide resolved
src/main/java/io/github/thebusybiscuit/sensibletoolbox/utils/IntRange.java Outdated Show resolved Hide resolved
src/main/java/io/github/thebusybiscuit/sensibletoolbox/utils/IntRange.java Outdated Show resolved Hide resolved
src/main/java/io/github/thebusybiscuit/sensibletoolbox/utils/IntRange.java Outdated Show resolved Hide resolved
src/main/java/io/github/thebusybiscuit/sensibletoolbox/utils/WordUtils.java Outdated Show resolved Hide resolved
src/main/java/io/github/thebusybiscuit/sensibletoolbox/utils/WordUtils.java Show resolved Hide resolved
src/main/java/io/github/thebusybiscuit/sensibletoolbox/utils/WordUtils.java Show resolved Hide resolved
Sefiraat
Sefiraat reviewed Jun 30, 2022
View reviewed changes
Copy link
Member
@Sefiraat Sefiraat left a comment

Choose a reason for hiding this comment

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

I think I'm happy with all of this now. I would like cookie's opinion on the WordUtils class as I'm not entirely sure about that one (but do not have a better suggestion outside of reinventing the wheel).

The only reason im not marking this as approved now is I want to build and test after work today :)

@TheBusyBiscuit TheBusyBiscuit self-assigned this Jul 2, 2022
@Sefiraat
Copy link
Member
Sefiraat commented Jul 9, 2022

6 days later....

Please try to remember that people have their own real lives, issues and work/school. Destructive comments like this are not useful in any way, shape or form and are simply not welcome. I cannot speak for cookie but I will not receive any comment like this well.

@ghost
Copy link
ghost commented Jul 28, 2022

Are you update this 1.19,1?
Thank you !

@TerslenK
Copy link

any news?

@Bunnky Bunnky changed the title 1.19 successful load 1.19-1.20 successful load Jun 19, 2023
@Bunnky
Copy link
Author
Bunnky commented Jun 19, 2023 •
edited
Loading

#751a7fa Is a 1.20 successful load. Like the previous, this is only for basic functionality.

@Bunnky Bunnky requested a review from Sefiraat June 19, 2023 11:27
@Bunnky Bunnky closed this by deleting the head repository Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheBusyBiscuit TheBusyBiscuit Awaiting requested review from TheBusyBiscuit

@Sefiraat Sefiraat Awaiting requested review from Sefiraat

Assignees

@TheBusyBiscuit TheBusyBiscuit

@Sefiraat Sefiraat

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Bunnky @Sefiraat @TerslenK @TheBusyBiscuit