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.

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

Respawn Anchor Support #4855

Merged
merged 12 commits into from Dec 28, 2022
Merged

Respawn Anchor Support #4855

merged 12 commits into from Dec 28, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jun 30, 2022

Description

Added respawn anchor elements.


Target Minecraft Versions: 1.16+
Requirements: Minecraft Version 1.16+
Related Issues: #4726

@AyhamAl-Ali AyhamAl-Ali added the feature Pull request adding a new feature. label Jun 30, 2022
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

Amazing PR! Thank you for your contribution :)

First pass (not tested yet), a second one will come after these RCs are done and another reviewer would be appreciated.

Mostly simple formatting stuff.
Also you should add Documentation Annotations for each class see other elements for examples

I didn't suggest changes on duplicated parts so make sure to apply the formatting style on all places.

It would be better to split each element in its own PR for faster reviews and to get them merged probably faster.

Well done 👏

src/main/java/ch/njol/skript/expressions/ExprCharges.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprCharges.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprCharges.java Outdated Show resolved Hide resolved
@TheLimeGlass
Copy link
Collaborator

We reviewed at the same time, so there are some duplicates there. Sorry about that. When you resolve the same underlying request, you can mark both as resolved.

Thank you for the pull request!

Copy link
Collaborator

@TheLimeGlass TheLimeGlass 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, nice work!

@TheLimeGlass TheLimeGlass requested a review from AyhamAl-Ali June 30, 2022 22:22
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Formatting and a few missing things

Overall nice work!!

src/main/java/ch/njol/skript/expressions/ExprCharges.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprCharges.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprCharges.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprCharges.java Outdated Show resolved Hide resolved
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

Well done so far :)
Just another minor changes

Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

Well done and thank you for doing the Requested Changes 👌

@ghost ghost requested a review from APickledWalrus October 10, 2022 01:16
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

A few minor formatting things, but won't block approval :)

Nice work! :D

Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
hotpocket184 and others added 3 commits December 26, 2022 15:13
@TheLimeGlass TheLimeGlass merged commit eb645b6 into SkriptLang:master Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants