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.

Sign up for GitHub

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

Jump to bottom

Optional player for SlimefunItemSpawnEvent #3998

Merged
merged 5 commits into from
Dec 6, 2023
Merged

Optional player for SlimefunItemSpawnEvent #3998

merged 5 commits into from
Dec 6, 2023

Conversation

TheSilentPro
Copy link
Contributor

@TheSilentPro TheSilentPro commented Oct 14, 2023 •
edited
Loading

Description

This adds Optional player field for SlimefunItemSpawnEvent.
Some of the spawn reasons are tied to a player and there is not way to get it currently.

Proposed changes

This adds a new constructor to the class that accepts a player field. This field is null by default.

Related Issues (if applicable)

n/a

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
  • I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.20.*).
  • I followed the existing code standards and didn't mess up the formatting.
  • I did my best to add documentation to any public classes or methods I added.
  • I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
  • I added sufficient Unit Tests to cover my code.

@TheSilentPro TheSilentPro requested a review from a team as a code owner October 14, 2023 23:41
@github-actions
Copy link
Contributor

Pro Tip!
You can help us label your Pull Requests by using the following branch naming convention next time you create a pull request. ❤️

Branch naming convention Label
feature/** 🎈 Feature
fix/** ✨ Fix
chore/** 🧹 Chores
api/** 🔧 API
performance/** 💡 Performance Optimization
compatibility/** 🤝 Compatibility
If your changes do not fall into any of these categories, don't worry. You can just ignore this message in that case! 👀

@TheSilentPro
Copy link
Contributor Author

I forgot to make a new branch and comited to master by accident. Sorry

@github-actions
Copy link
Contributor

github-actions bot commented Oct 14, 2023 •
edited
Loading

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: 4ed36ccd

https://preview-builds.walshy.dev/download/Slimefun/3998/4ed36ccd

Note: This is not a supported build and is only here for the purposes of testing.
Do not run this on a live server and do not report bugs anywhere but this PR!

@J3fftw1 J3fftw1 added 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. 🎈 Feature This Pull Request adds a new feature. 🔧 API This Pull Request introduces API changes. labels Oct 14, 2023
TheBusyBiscuit
TheBusyBiscuit reviewed Oct 15, 2023
View reviewed changes
Copy link
Member

@TheBusyBiscuit TheBusyBiscuit left a comment

Choose a reason for hiding this comment

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

Looks good but player might be a bit too ambiguous. One could think that that only this player can pick it up or whatever. Perhaps "causedBy" might be a better name here.

But it is also fine to stay as is, just a suggestion. 👍🏻

src/main/java/io/github/thebusybiscuit/slimefun4/api/events/SlimefunItemSpawnEvent.java Outdated Show resolved Hide resolved
@TheSilentPro @TheBusyBiscuit
Update src/main/java/io/github/thebusybiscuit/slimefun4/api/events/Sl…
bd4ca3e 
…imefunItemSpawnEvent.java


Mark optional as nonnull

Co-authored-by: TheBusyBiscuit <[email protected]>
@TheSilentPro
Copy link
Contributor Author

Looks good but player might be a bit too ambiguous. One could think that that only this player can pick it up or whatever. Perhaps "causedBy" might be a better name here.

But it is also fine to stay as is, just a suggestion. 👍🏻

I think player would fit fine, either way theres a javadoc on it.

J3fftw1
J3fftw1 previously approved these changes Oct 15, 2023
View reviewed changes
@TheSilentPro
Copy link
Contributor Author

Resolved

JustAHuman-xD
JustAHuman-xD approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

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

LGTM

JustAHuman-xD
JustAHuman-xD approved these changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

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

LGTM, only thing that you could but don't need to change is in some places you use e.getPlayer() which is called multiple times within that code block so you could define a variable for it but it's not a big deal

Sfiguz7
Sfiguz7 approved these changes Dec 6, 2023
View reviewed changes
Copy link
Member

@Sfiguz7 Sfiguz7 left a comment

Choose a reason for hiding this comment

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

Justin's point makes sense but I agree it's fine as is. Just acknowledging before I merge

@Sfiguz7 Sfiguz7 merged commit ddbe8ae into Slimefun:master Dec 6, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JustAHuman-xD JustAHuman-xD JustAHuman-xD approved these changes

@Sfiguz7 Sfiguz7 Sfiguz7 approved these changes

@J3fftw1 J3fftw1 J3fftw1 left review comments

@TheBusyBiscuit TheBusyBiscuit Awaiting requested review from TheBusyBiscuit

Assignees

@Sfiguz7 Sfiguz7

@JustAHuman-xD JustAHuman-xD

Labels
🔧 API This Pull Request introduces API changes. 🎈 Feature This Pull Request adds a new feature. 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@TheSilentPro @TheBusyBiscuit @Sfiguz7 @J3fftw1 @JustAHuman-xD @PBoki