Conversation
Paper exposes the internals we need directly. Rather than muddle around and hope the enchantments haven't changed, use Paper's API if not baked in. Probably will rewrite this to directly use the API rather than via MethodHandles, but I don't know if Paper has deprecated other areas so I'm leery of swapping yet.
Tests are horribly broken, of course
This test isn't as useful as it was before - all the values are data-driven from server internals, and the server internals aren't loaded because it's just the API. It does still serve to catch some issues, but it can miss some.
Unlike the enchantability test, the repair material test is purely data-driven and requires loading the server.
Increased breadth of things mocked by default, updated to current internals Fixed missed tags for enchantability
It's no longer a heavily version-specific thing, so a helper method is a bit overkill.
The test is a bit of a nothing, honestly. It more tests the API than anything. Probably should have deleted it rather than invest time in fixing it.
This allows for better backwards compatibility in the future
As Paper has delegated ItemStack to a backing implementation and our backing implementation is a mock, the .equals method is not available directly.
Use customName in place of display name Prefer ItemType over material
Not quite as version-independent as enchantability yet, but an improvement.
Removed old enchantment hacks in favor of proper mocks
Can you tell that I am bad at remembering to use the debugger? Oops. I did get better during the process of this, I promise.
This wasn't necessary to my knowledge due to how Dependabot works, but modernizing does remove a runner from the process, which is good. Need to update PlanarActions later and move this over to it. Can probably abstract the semver reqs out to an input list or something.
Migrating away from Bukkit constructs
Sonar is still going to be unhappy about it existing, but now it can be less unhappy.
A little less efficient, but also only done in testing.
Hopefully Paper will add ItemStack#getItemType or similar eventually.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Spigot seems to be falling behind in terms of upkeeping old APIs. If https://hub.spigotmc.org/jira/browse/SPIGOT-7838 gets resolved in a useful fashion it may be worth reconsidering, but they're years behind Paper in terms of exposing enchantment details.
Still need to consider Paperweight for easier test writing and possibly data-driven code generation. As that requires a swap to Gradle, I'm not ready to do it yet, but it should likely be done before a full 3.0.0 release.