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

Improve default comparator resolution #7287

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

Conversation

APickledWalrus
Copy link
Member

Description

This PR aims to fix the root issue that motivated PRs such as #7251. I have not reverted those changes, as it does not hurt to register the comparator directly. This needs further testing, but it should avoid the issues of allowing comparisons of unrelated classes that happen to share an interface (e.g. PersistentDataHolders).


Target Minecraft Versions: any
Requirements: none
Related Issues: none

@APickledWalrus APickledWalrus added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. labels Dec 19, 2024
@Pikachu920
Copy link
Member

should we still retain the original check too? can we add a test?

@APickledWalrus
Copy link
Member Author

should we still retain the original check too? can we add a test?

i'm not sure the original check ended up doing much (hence the need for this PR). if there were any cases where it was, they should still work with this PR, just with the classinfo resolution instead.
as for a test, the PR I mentioned in the description (7251) resolves the existing occurrences of this issue, so I'm not sure I could test it without reverting those changes.

@sovdeeth sovdeeth added the 2.10 Targeting a 2.10.X version release label Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.10 Targeting a 2.10.X version release bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants