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

Adding entity expressions & conditions #7088

Open
wants to merge 31 commits into
base: dev/feature
Choose a base branch
from
Open

Adding entity expressions & conditions #7088

wants to merge 31 commits into from
+234 −5

Conversation

Nuutrai
Copy link

@Nuutrai Nuutrai commented Sep 18, 2024 •
edited
Loading

Description

I've added most of the remaining features on the entity section (forgot to mention that when initially posting) todo list, however I'll continue trying to work on the ones remaining

Target Minecraft Versions: any
Requirements: none
Related Issues: Todo List (#4185)

Let me know if you have any suggestions such as syntax (I know syntax will probably want to be changed).

@Nuutrai Nuutrai changed the base branch from master to dev/feature September 18, 2024 01:08
@Fusezion

This comment was marked as resolved.

Sign in to view
@Nuutrai
Copy link
Author

Nuutrai commented Sep 18, 2024 •
edited
Loading

I found it, but thank you!

cheeezburga
cheeezburga requested changes Sep 18, 2024
View reviewed changes
Copy link
Member

@cheeezburga cheeezburga left a comment

Choose a reason for hiding this comment

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

On top of all the comments, would love to see tests for all of these!

src/main/java/ch/njol/skript/conditions/CondFromMobSpawner.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondFromMobSpawner.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondFromMobSpawner.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondFromMobSpawner.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondFromMobSpawner.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsTicking.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsTicking.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprCustomNameVisibility.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprMaxFireTicks.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprMaxFireTicks.java Outdated Show resolved Hide resolved
@cheeezburga cheeezburga added enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature Pull request adding a new feature. and removed feature Pull request adding a new feature. labels Sep 18, 2024
Efnilite
Efnilite requested changes Sep 18, 2024
View reviewed changes
src/main/java/ch/njol/skript/conditions/CondFromMobSpawner.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondFromMobSpawner.java Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondFromMobSpawner.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsCustomNameVisible.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsCustomNameVisible.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsTicking.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsCustomNameVisible.java Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsTicking.java Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprCustomNameVisibility.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprMaxFireTicks.java Outdated Show resolved Hide resolved
Nuutrai and others added 2 commits September 18, 2024 11:15
@Nuutrai @Efnilite
Apply suggestions
ccd4746 
Co-authored-by: Efy <[email protected]>
@Nuutrai
Changes + fixes
39e243c 
Added EffCustomName
Added EffCustomName test
Removed ExprCustomNameVisibility.java
Fixed ParseResult import
cheeezburga
cheeezburga requested changes Sep 19, 2024
View reviewed changes
Copy link
Member

@cheeezburga cheeezburga left a comment

Choose a reason for hiding this comment

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

Just needs some tests for the other syntax still

src/main/java/ch/njol/skript/effects/EffCustomName.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffCustomName.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffCustomName.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffCustomName.java Outdated Show resolved Hide resolved
@sovdeeth
Copy link
Member

Please do not add any more syntaxes than what you have to this pr, and for future additions try to keep prs focused on a single topic. Working on that todo list is great but we want the prs to be small and focused so that issues with one part don't slow down other parts that are good to go!

@Nuutrai
Copy link
Author

Nuutrai commented Sep 19, 2024

Please do not add any more syntaxes than what you have to this pr, and for future additions try to keep prs focused on a single topic. Working on that todo list is great but we want the prs to be small and focused so that issues with one part don't slow down other parts that are good to go!

Alrighty! I'll work on the test for CondIsTicking (since it's a little hard to test CondFromMobSpawner), then I'll mark the pr as ready for review. (Let me know if any of the other things need tests or anything!)

@Nuutrai Nuutrai marked this pull request as ready for review September 19, 2024 14:13
sovdeeth
sovdeeth requested changes Sep 19, 2024
View reviewed changes
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

looks pretty good!

src/main/java/ch/njol/skript/conditions/CondFromMobSpawner.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffCustomName.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprMaxFireTicks.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprMaxFireTicks.java Outdated Show resolved Hide resolved
@Nuutrai
Copy link
Author

Nuutrai commented Sep 19, 2024

Whoops, forgot to rename ExprMaxFireTicks

Efnilite
Efnilite requested changes Sep 23, 2024
View reviewed changes
src/main/java/ch/njol/skript/conditions/CondFromMobSpawner.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsCustomNameVisible.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsCustomNameVisible.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondFromMobSpawner.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsTicking.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffCustomName.java Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprFireTicks.java Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprFireTicks.java Outdated Show resolved Hide resolved
src/test/skript/tests/syntaxes/conditions/CondIsCustomNameVisible.sk Show resolved Hide resolved
src/test/skript/tests/syntaxes/conditions/CondIsTicking.sk Show resolved Hide resolved
@Nuutrai Nuutrai requested a review from sovdeeth October 3, 2024 15:51
@cheeezburga cheeezburga added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Oct 9, 2024
@Nuutrai Nuutrai changed the title Adding features to help complete the todo list Adding entity expressions & conditions Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fusezion Fusezion Fusezion left review comments

@ShaneBeee ShaneBeee ShaneBeee left review comments

@cheeezburga cheeezburga cheeezburga approved these changes

@Efnilite Efnilite Efnilite approved these changes

@sovdeeth sovdeeth sovdeeth approved these changes

Assignees
No one assigned
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Nuutrai @Fusezion @sovdeeth @ShaneBeee @Efnilite @cheeezburga