Alexandra-Myers / Combatify

GNU General Public License v3.0
21 stars 7 forks source link

Antipatterns in ItemMixin #52

Closed Daomephsta closed 1 year ago

Daomephsta commented 1 year ago

ItemMixin.getUseDuration() replicates Item.getUseDuration() with a cancellable @Inject to insert a small section of mod code. This is a very problematic way to implement functionality because:

  1. Any injected handlers after a handler using this pattern will silently fail to run
  2. It's hard to maintain. Every MC version update requires re-copying and re-adapting the code to cancellable @Inject 3, It's legally dubious at best. Minecraft's code is proprietary, so avoid copying it if at all possible.

@Overwrite would be a better solution here. It is in fact more compatible than the current approach, because it will not cause other injectors to silently fail. There's probably a way to do without it, but it would be an improvement.

Also should be following Mixin best practices:

zOnlyKroks commented 1 year ago

Will change that to an override once I find time

zOnlyKroks commented 1 year ago

Also gonna try to remove the aw`s

Daomephsta commented 1 year ago

Will change that to an override once I find time

Assuming this is a typo, but just in case, @Overwrite is different from overriding a method. Overriding a method in a mixin has a similar effect to overwriting, but reobfuscation will often fail, and you lose Mixin's overwrite conflict detection.

Alexandra-Myers commented 1 year ago

'Twas a typo

Alexandra-Myers commented 1 year ago

he just does that sometimes

zOnlyKroks commented 1 year ago

Will change that to an override once I find time

Assuming this is a typo, but just in case, @Overwrite is different from overriding a method. Overriding a method in a mixin has a similar effect to overwriting, but reobfuscation will often fail, and you lose Mixin's overwrite conflict detection.

Yea did a big fat typo. But fixed in the new branch

zOnlyKroks commented 1 year ago

Along with a bunch of other stuff

zOnlyKroks commented 1 year ago

Fixed in new branch