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

Housekeeping: deprecate, replace or remove archaic utilities classes. #6670

Open
wants to merge 8 commits into
base: dev/feature
Choose a base branch
from

Conversation

Moderocky
Copy link
Member

@Moderocky Moderocky commented May 8, 2024

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 deprecated Checker<T> because that is now a predicate itself.

The process is as follows:

  1. Make the legacy class extend its proper replacement (e.g. Checker<T> extends Predicate<T>)
  2. Forward a default method call from the proper method to the legacy method
  3. Replace uses of the legacy class with the proper class (where possible)
  4. Deprecate the legacy class and mark it for removal.

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

@Moderocky Moderocky added enhancement Feature request, an issue about something that could be improved, or a PR improving something. 2.9 Targeting a 2.9.X version release housekeeping Housekeeping-related issue or task. breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) labels May 8, 2024
@Moderocky Moderocky force-pushed the feature/deprecate-outdated-utils branch from eccdb57 to ebbf61b Compare May 9, 2024 08:01
@Moderocky Moderocky marked this pull request as ready for review May 30, 2024 08:04
Moderocky and others added 8 commits May 30, 2024 09:07
* 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
@Moderocky Moderocky force-pushed the feature/deprecate-outdated-utils branch from ebbf61b to 24c10f9 Compare May 30, 2024 08:08
@Moderocky Moderocky requested review from APickledWalrus, sovdeeth and UnderscoreTud and removed request for APickledWalrus and sovdeeth May 30, 2024 08:08
Copy link
Member

@APickledWalrus APickledWalrus left a 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> {
Copy link
Member

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

Copy link
Member Author

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)).

Copy link
Member

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

Comment on lines +70 to +73
if (replace)
Files.move(from.toPath(), to.toPath(), StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE);
else
Files.move(from.toPath(), to.toPath(), StandardCopyOption.ATOMIC_MOVE);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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> {
Copy link
Member

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 {
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.9 Targeting a 2.9.X version release breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) enhancement Feature request, an issue about something that could be improved, or a PR improving something. housekeeping Housekeeping-related issue or task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants