-
-
Notifications
You must be signed in to change notification settings - Fork 354
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
Housekeeping: deprecate, replace or remove archaic utilities classes. #6670
base: dev/feature
Are you sure you want to change the base?
Conversation
eccdb57
to
ebbf61b
Compare
* Mark VectorMath as internal * Replace usages
* Make Setter a Consumer. * Remove Setter from Option. * Remove setter from entry validator. * Remove setter from enum entry validator. * Remove setter from parsed entry validator. * Remove setter from parsed section validator.
* Make checker a Java predicate. * Deprecate unused Njol predicate for removal. * Make checker extensions use predicate directly. * Remove checker from expression. * Remove checker from utils. * Deprecate Checker for removal. * Abstract checker to predicate from simple syntax * Remove remaining uses of checker.
* Mark Validate as internal * Schedule for removal + change usages * Use preconditions
ebbf61b
to
24c10f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My primary concerns with these changes are that the Java-equivalent classes have methods for function composition. For any classes implementing these now deprecated classes, they now have these methods which may not be properly implemented (or work how one might expect)
@@ -52,7 +53,7 @@ | |||
* {@link PropertyCondition#register(Class, String, String)}, be aware that there can only be two patterns - | |||
* the first one needs to be a non-negated one and a negated one. | |||
*/ | |||
public abstract class PropertyCondition<T> extends Condition implements Checker<T> { | |||
public abstract class PropertyCondition<T> extends Condition implements ch.njol.util.Checker<T>, Predicate<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of Predicate
here raises some fun questions. It contains several default methods that allow you to modify the predicate. I'm not sure how applicable they are, but we might wish to override them or disallow them completely.
For example, should negate()
function the same as setNegated(!isNegated())
? It might be the case that negate()
expects a new object rather than the same one (which we can't really promise)
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/function/Predicate.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No walrus they don't modify the predicate they retain adjusted wrappers for it, e.g. the negation of o -> true
would be (!(o -> true))
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that the methods such as negate
are a bit unexpected on PropertyCondtion, especially given we already have setNegated
if (replace) | ||
Files.move(from.toPath(), to.toPath(), StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); | ||
else | ||
Files.move(from.toPath(), to.toPath(), StandardCopyOption.ATOMIC_MOVE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (replace) | |
Files.move(from.toPath(), to.toPath(), StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); | |
else | |
Files.move(from.toPath(), to.toPath(), StandardCopyOption.ATOMIC_MOVE); | |
if (replace) { | |
Files.move(from.toPath(), to.toPath(), StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); | |
} else { | |
Files.move(from.toPath(), to.toPath(), StandardCopyOption.ATOMIC_MOVE); | |
} |
|
||
import java.util.function.Predicate; | ||
|
||
public interface NullableChecker<T> extends ch.njol.util.Checker<T>, Predicate<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we able to override test
for its parameter to be nullable?
public class VectorMath { | ||
@Deprecated | ||
@ApiStatus.ScheduledForRemoval | ||
public final class VectorMath { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason to remove this class? It seems like there is now some duplicated code in the vector elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from @kiip1 's PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although a few duplicate cases have been introduced by these changes, I still feel moving the static methods to where they're used is beneficial, besides the content of these methods won't ever change so the duplication of the methods doesn't matter as much.
Another argument for this change is that there are many more cases where it isn't duplicated, so you'd either be left with unused methods in VectorMath (in case you keep all methods) or a very minimal VectorMath (in case you remove the duplicate methods). I prefer removing the entire utility class over keeping a partial class or having unused code.
At the end of the day you do have a fair argument, and there are multiple ways to solve the problem and I chose the solution which removes the entire class, at the cost of some duplicates.
Description
A lot of utility types were left over from Java 6.
Most of these weren't used at all, but a few are still being used in place of the proper equivalent in legacy code.
This pull request is a bulk group task to deprecate all of these for removal, and to replace internal uses of them all with a better thing.
Where possible, compatibility is perfectly preserved, but in some obscure cases (e.g. constructors) parameters have to be changed.
Legacy libraries
Some legacy libraries have been removed, moved, or just deprecated for removal.
Stubs for each one have been left behind (so technically nothing still using them will break).
Legacy functional interfaces
We have a legacy equivalent of Predicate, Consumer and Function, along with some others.
A lot of these can be replaced without any breaking changes. For some, where a method signature has changed, a plugin extending the method would need to be recompiled but wouldn't require any changes: for example, a method accepting
Predicate<T>
will also accept the deprecatedChecker<T>
because that is now a predicate itself.The process is as follows:
Checker<T> extends Predicate<T>
)This is a branch merge request!
Individual pull requests were made by contributors to its feature branch: this can be merged as-is to preserve the Git change history.
Target Minecraft Versions: any
Requirements: none
Related Issues: none