Skip to content

Conversation

NoyException
Copy link

When using a custom merchant, it's common to want to know whether a recipe is just used by listening PlayerPurchaseEvent. However, if we couldn't know which merchant players are trading with, we can't reach that goal.

So I just simply add the approach.

@NoyException NoyException requested a review from a team as a code owner September 7, 2025 16:43
@github-project-automation github-project-automation bot moved this to Awaiting review in Paper PR Queue Sep 7, 2025
Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

Sounds sensible, however now we are kinda doubling the #getVillager method in the PlayerTradeEvent as #getMerchant yields the same.

It might be beneficial to mark the #getVillager method as obsolete, overide the #getMerchant method to yield an AbstractVillager in PlayerTradeEvent and then have #getVillager link to it.

@NoyException
Copy link
Author

Sounds sensible, however now we are kinda doubling the #getVillager method in the PlayerTradeEvent as #getMerchant yields the same.

It might be beneficial to mark the #getVillager method as obsolete, overide the #getMerchant method to yield an AbstractVillager in PlayerTradeEvent and then have #getVillager link to it.

You're absolutely right! I have made the changes as you said.

Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

Neither of the events should be using jetbrains null-annotations, they are using jspecify already so should be fine without NotNull

@github-project-automation github-project-automation bot moved this from Awaiting review to Changes required in Paper PR Queue Sep 9, 2025
@NoyException
Copy link
Author

Neither of the events should be using jetbrains null-annotations, they are using jspecify already so should be fine without NotNull

Sure! I've corrected those. Could you please review again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Changes required
Development

Successfully merging this pull request may close these issues.

2 participants