Change creeper max fuse and no damage ticks to timespan #5225

kiip1 · 2022-11-27T21:01:52Z

Description

Change creeper max fuse and no damage ticks from amount of ticks to timespan, although the former was logical due to it being the property in Bukkit, it doesn't align with the rest of Skript's syntax, therefore it should be timespan instead.

Related Issues:

Support for timespan in no damage ticks #5197

Moderocky · 2022-11-27T21:06:41Z

This will be a breaking change for users without sufficient justification. You could support the original format as an additional pattern.

src/test/skript/tests/syntaxes/expressions/ExprCreeperMaxFuseTicks.sk

src/test/skript/tests/syntaxes/expressions/ExprNoDamageTicks.sk

TheLimeGlass · 2022-11-28T21:36:50Z

This syntax can also support a primed tnt in the future, so it's a scuffed addition but we have to deal with it now.

I like this syntax change.

APickledWalrus

Looks pretty good. Might be worth discussing Kenzie's proposal.

src/main/java/ch/njol/skript/expressions/ExprCreeperMaxFuseTicks.java

AyhamAl-Ali

Nice PR ⚡

src/main/java/ch/njol/skript/expressions/ExprNoDamageTicks.java

src/main/java/ch/njol/skript/expressions/ExprCreeperMaxFuseTicks.java

src/main/java/ch/njol/skript/expressions/ExprNoDamageTicks.java

Co-authored-by: Ayham Al Ali <[email protected]>

src/main/java/ch/njol/skript/expressions/ExprCreeperMaxFuseTicks.java

src/main/java/ch/njol/skript/expressions/ExprNoDamageTicks.java

src/main/java/ch/njol/skript/expressions/ExprCreeperMaxFuseDuration.java

Co-authored-by: LimeGlass <[email protected]>

APickledWalrus

I'd be happy to take this over if you are no longer interested in working on it.

APickledWalrus · 2024-05-31T15:07:28Z

src/main/java/ch/njol/skript/expressions/ExprCreeperMaxFuseDuration.java

+
+	@Override
+	public Timespan convert(LivingEntity entity) {
+		return Timespan.fromTicks_i(entity instanceof Creeper ? ((Creeper) entity).getMaxFuseTicks() : 0);


It's different from the old expression behavior, but I'm wondering if it would be better to return null here if it is not a creeper.

There is no "old expression behaviour" here (refer to other comment) so it would be good to return null.

APickledWalrus · 2024-05-31T15:08:26Z

src/main/java/ch/njol/skript/expressions/ExprCreeperMaxFuseDuration.java

+	@Override
+	@Nullable
+	public Class<?>[] acceptChange(ChangeMode mode) {
+		return (mode != ChangeMode.REMOVE_ALL) ? CollectionUtils.array(Timespan.class) : null;


I think it would be better to use a switch here and check for the accepted modes rather than against.

APickledWalrus · 2024-05-31T15:10:05Z

src/main/java/ch/njol/skript/expressions/ExprCreeperMaxFuseDuration.java

+					break;
+				case DELETE:
+				case SET:
+					creeper.setMaxFuseTicks(Math.max(amount, 0));


Suggested change

creeper.setMaxFuseTicks(Math.max(amount, 0));

creeper.setMaxFuseTicks(amount);

amount should always be greater than 0 here

APickledWalrus · 2024-05-31T15:11:04Z

src/main/java/ch/njol/skript/expressions/ExprCreeperMaxFuseTicks.java

Might be worth keeping this for backwards compatibility like ExprNoDamageTicks

APickledWalrus · 2024-05-31T15:11:19Z

src/main/java/ch/njol/skript/expressions/ExprCreeperMaxFuseDuration.java

+
+	@Override
+	protected String getPropertyName() {
+		return "creeper max fuse time";


Suggested change

return "creeper max fuse time";

return "creeper maximum fuse time";

APickledWalrus · 2024-05-31T15:12:49Z

src/test/skript/tests/syntaxes/expressions/ExprNoDamageDuration.sk

+	assert {_m}'s invulnerability time is 1 tick with "no damage time add ##2 failed"
+	delete {_m}'s invulnerability time
+	assert {_m}'s invulnerability time is 0 seconds with "no damage time delete failed"
+	delete {_m}


Suggested change

delete {_m}

delete the entity within {_m}

APickledWalrus · 2024-05-31T15:14:02Z

src/main/java/ch/njol/skript/expressions/ExprNoDamageDuration.java

+public class ExprNoDamageDuration extends SimplePropertyExpression<LivingEntity, Timespan> {
+
+	static {
+		register(ExprNoDamageDuration.class, Timespan.class, "(invulnerability|no damage) (time|duration)", "livingentities");


any reason invincibility was removed?

My change was due to it not being refered to as "invincibility" in Bukkit, but for consistency it can be re-added.

APickledWalrus · 2024-05-31T15:14:25Z

src/main/java/ch/njol/skript/expressions/ExprNoDamageTicks.java

+		if (!super.init(exprs, matchedPattern, isDelayed, parseResult))
+			return false;
+
+		Skript.warning("Switch from deprecated 'no damage ticks' to 'no damage duration', as the deprecated expression will be removed in 2.8.");


Could be good to use the deprecation warning: #6549

APickledWalrus · 2024-05-31T15:51:58Z

src/main/java/ch/njol/skript/expressions/ExprNoDamageDuration.java

+
+	@Override
+	protected String getPropertyName() {
+		return "no damage time";


Suggested change

return "no damage time";

return "no damage duration";

since we refer to it as duration in the docs

APickledWalrus · 2024-05-31T15:52:44Z

src/main/java/ch/njol/skript/expressions/ExprNoDamageDuration.java

+@Description("How much time an entity is invulnerable for.")
+@Examples({"on damage:",
+		"\tset victim's invulnerability time to 1 second # victim will not take damage for the next second"})
+@Since("2.5, INSERT VERSION (Use Timespan)")


Suggested change

@Since("2.5, INSERT VERSION (Use Timespan)")

@Since("INSERT VERSION")

I'm tempted to say this since it's essentially a different syntax (though it returns the same property). I'd like thoughts though.

kiip1

@APickledWalrus feel free to take this PR over :)

kiip1 · 2024-06-09T17:15:13Z

src/main/java/ch/njol/skript/expressions/ExprCreeperMaxFuseTicks.java

-public class ExprCreeperMaxFuseTicks extends SimplePropertyExpression<LivingEntity, Long> {
-
-	static {
-		if(Skript.methodExists(LivingEntity.class, "getMaxFuseTicks"))


This syntax has never actually worked since LivingEntity doesn't have getMaxFuseTicks

kiip1 · 2024-06-09T17:15:56Z

src/main/java/ch/njol/skript/expressions/ExprCreeperMaxFuseDuration.java

+
+	@Override
+	public Timespan convert(LivingEntity entity) {
+		return Timespan.fromTicks_i(entity instanceof Creeper ? ((Creeper) entity).getMaxFuseTicks() : 0);


There is no "old expression behaviour" here (refer to other comment) so it would be good to return null.

kiip1 · 2024-06-09T17:17:33Z

src/main/java/ch/njol/skript/expressions/ExprNoDamageDuration.java

+public class ExprNoDamageDuration extends SimplePropertyExpression<LivingEntity, Timespan> {
+
+	static {
+		register(ExprNoDamageDuration.class, Timespan.class, "(invulnerability|no damage) (time|duration)", "livingentities");


My change was due to it not being refered to as "invincibility" in Bukkit, but for consistency it can be re-added.

kiip1 · 2024-06-09T17:18:00Z

src/test/skript/tests/syntaxes/expressions/ExprCreeperMaxFuseDuration.sk

+	delete {_m}
+	delete {_n}


PR was before this syntax was released :)

Moderocky

Branch conflicts need resolving.

Change creeper max fuse and no damage ticks to timespan and add tests

637eda3

kiip1 added 2 commits November 27, 2022 22:07

Fix reset changer and incorrect Skript#methodExists parameter

a1804ad

Merge branch 'master' into ticks-to-timespan

7fbe22e

TheLimeGlass requested changes Nov 28, 2022

View reviewed changes

src/test/skript/tests/syntaxes/expressions/ExprCreeperMaxFuseTicks.sk Outdated Show resolved Hide resolved

src/test/skript/tests/syntaxes/expressions/ExprNoDamageTicks.sk Outdated Show resolved Hide resolved

TheLimeGlass added enhancement Feature request, an issue about something that could be improved, or a PR improving something. breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) labels Nov 28, 2022

Cleanup tests

d8e948e

TheLimeGlass approved these changes Nov 28, 2022

View reviewed changes

TheLimeGlass requested a review from Moderocky November 28, 2022 22:31

APickledWalrus reviewed Dec 10, 2022

View reviewed changes

src/main/java/ch/njol/skript/expressions/ExprCreeperMaxFuseTicks.java Outdated Show resolved Hide resolved

kiip1 added 2 commits December 12, 2022 21:58

Fix unneeded Math#max

301a03f

Merge branch 'master' into ticks-to-timespan

a1062db

kiip1 requested review from APickledWalrus and removed request for Moderocky December 12, 2022 21:05

AyhamAl-Ali requested changes Dec 13, 2022

View reviewed changes

kiip1 and others added 2 commits December 13, 2022 16:47

Update src/main/java/ch/njol/skript/expressions/ExprNoDamageTicks.java

d2b2410

Co-authored-by: Ayham Al Ali <[email protected]>

Apply suggested changes

5abecbe

kiip1 requested review from AyhamAl-Ali and removed request for APickledWalrus December 13, 2022 16:06

Merge branch 'master' into ticks-to-timespan

a219a6a

AyhamAl-Ali approved these changes Dec 30, 2022

View reviewed changes

TheLimeGlass requested review from Moderocky and APickledWalrus January 9, 2023 17:22

kiip1 added 2 commits January 28, 2023 16:07

Merge branch 'master' into ticks-to-timespan

2dd1ba2

Merge branch 'master' into ticks-to-timespan

1c59117

TheLimeGlass removed the request for review from Moderocky February 4, 2023 03:54

TheLimeGlass requested a review from UnderscoreTud February 16, 2023 07:45

UnderscoreTud reviewed Feb 17, 2023

View reviewed changes

src/main/java/ch/njol/skript/expressions/ExprCreeperMaxFuseTicks.java Outdated Show resolved Hide resolved

kiip1 added 3 commits February 17, 2023 17:55

Merge branch 'master' into ticks-to-timespan

15e2a74

Support legacy

61ce69f

Merge branch 'master' into ticks-to-timespan

e1ce6f8

kiip1 mentioned this pull request Mar 26, 2023

Fix test crashing the test server #5547

Closed

kiip1 added 2 commits April 8, 2023 15:10

Merge branch 'master' into ticks-to-timespan

d1b4d08

Merge branch 'master' into ticks-to-timespan

88e62eb

TheLimeGlass requested changes May 4, 2023

View reviewed changes

src/main/java/ch/njol/skript/expressions/ExprNoDamageTicks.java Outdated Show resolved Hide resolved

src/main/java/ch/njol/skript/expressions/ExprCreeperMaxFuseDuration.java Outdated Show resolved Hide resolved

Moderocky force-pushed the master branch 2 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59

Apply suggestions from code review

e15ffb9

Co-authored-by: LimeGlass <[email protected]>

sovdeeth added the 2.9 Targeting a 2.9.X version release label Mar 19, 2024

Update ExprNoDamageTicks.java

fa247bb

sovdeeth changed the base branch from master to dev/feature March 19, 2024 19:14

sovdeeth added 2 commits March 19, 2024 20:15

Merge branch 'dev/feature' into ticks-to-timespan

f7be39a

Fix removal annotation

606043f

sovdeeth requested review from TheLimeGlass and UnderscoreTud March 19, 2024 19:24

APickledWalrus requested changes May 31, 2024

View reviewed changes

kiip1 commented Jun 9, 2024

View reviewed changes

APickledWalrus self-assigned this Jun 20, 2024

sovdeeth removed the 2.9 Targeting a 2.9.X version release label Jun 28, 2024

TheLimeGlass approved these changes Jul 16, 2024

View reviewed changes

Moderocky reviewed Aug 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change creeper max fuse and no damage ticks to timespan #5225

Change creeper max fuse and no damage ticks to timespan #5225

kiip1 commented Nov 27, 2022

Moderocky commented Nov 27, 2022

TheLimeGlass commented Nov 28, 2022

APickledWalrus left a comment

AyhamAl-Ali left a comment

APickledWalrus left a comment

APickledWalrus May 31, 2024

kiip1 Jun 9, 2024

APickledWalrus May 31, 2024

APickledWalrus May 31, 2024

APickledWalrus May 31, 2024

APickledWalrus May 31, 2024

APickledWalrus May 31, 2024

APickledWalrus May 31, 2024

kiip1 Jun 9, 2024

APickledWalrus May 31, 2024

APickledWalrus May 31, 2024

APickledWalrus May 31, 2024

kiip1 left a comment

kiip1 Jun 9, 2024

kiip1 Jun 9, 2024

APickledWalrus Jun 10, 2024

kiip1 Jun 9, 2024

kiip1 Jun 9, 2024

kiip1 Jun 9, 2024

Moderocky left a comment

	creeper.setMaxFuseTicks(Math.max(amount, 0));
	creeper.setMaxFuseTicks(amount);

	return "creeper max fuse time";
	return "creeper maximum fuse time";

	@Since("2.5, INSERT VERSION (Use Timespan)")
	@Since("INSERT VERSION")

Change creeper max fuse and no damage ticks to timespan #5225

Are you sure you want to change the base?

Change creeper max fuse and no damage ticks to timespan #5225

Conversation

kiip1 commented Nov 27, 2022

Description

Moderocky commented Nov 27, 2022

TheLimeGlass commented Nov 28, 2022

APickledWalrus left a comment

Choose a reason for hiding this comment

AyhamAl-Ali left a comment

Choose a reason for hiding this comment

APickledWalrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiip1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Moderocky left a comment

Choose a reason for hiding this comment