Skip to content

Conversation

@modmuss50
Copy link
Member

Part 2 of cherry picking @Earthcomputer 's PR: #5118

Part 3 I will look at the comments on the orginal PR regarding removing unneeded locals, lets keep it focused and merge this one quickly to reduce conflicts.

@LlamaLad7
Copy link
Member

Personally I'm not entirely convinced, see my comments from the original PR:

  • Using names for argsOnly locals seems to me less stable than using ordinals / implicits, since it seems more likely for the internal code to change than the external api
  • Implicit mode seems potentially more stable than names when possible for normal locals, though I feel less strongly about this

If people have reason to disagree then we can discuss but I don't think "use name everywhere" is a sensible blanket strategy.
I suggested avoiding the locals where possible because that's definitely better, whereas a lot of these changes are dubiously better.

@modmuss50
Copy link
Member Author

Ah sorry, I must have missed that comment on the other PR.

Good point on argsOnly, I think I will change that.

I dont think stability across versions is the main goal here if something is renamed/chages its usually done for a reason, and more than likely may cause the mixin to behave differently.

@modmuss50 modmuss50 added the status: merge me please Pull requests that are ready to merge label Jan 28, 2026
@modmuss50 modmuss50 merged commit c82f046 into FabricMC:26.1 Jan 29, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup status: merge me please Pull requests that are ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

handleInteract Injection in ServerGamePacketListenerImplMixin Not Marked as cancellable

4 participants