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

⚒️ Enhance and Fix ExprAge #4854

Merged
merged 31 commits into from
Aug 6, 2022

Conversation

AyhamAl-Ali
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali commented Jun 30, 2022

Description

In this PR (continue of #4594):

  • Added support for entities
  • Added support for multiple blocks
  • Fixed an IAE when setting a block's age above its maximum or below 0
  • Added more examples
  • Improved description
  • Changed class name from ExprBlockAge to ExprAge
  • Removed class exist check due to legacy versions support drop

Target Minecraft Versions: 1.13+
Requirements: MC 1.13+
Related Issues:

  • Age expression #3068 (now fully supported. %itemtype% support is not needed and it doesn't have any kind if Ageable support, probably Snow thought of that due to how his code worked)

@AyhamAl-Ali AyhamAl-Ali added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Jun 30, 2022
@AyhamAl-Ali AyhamAl-Ali mentioned this pull request Jun 30, 2022
@AyhamAl-Ali AyhamAl-Ali added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Jun 30, 2022
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.

The changers should really support every ChangeMode as this is a number. I do this alot with Khoryl, so I have these on copy and paste. I have an implementation that makes this small and simpler, but this works for Skript conversion.

private void setAge(Object object, int age) {
	if (object instanceof Block) {
		Block block = ((Block) object);
		Ageable ageable = (Ageable) block.getBlockData();
		ageable.setAge(age);
		block.setBlockData(ageable);
	} else if (object instanceof org.bukkit.entity.Ageable) {
		((org.bukkit.entity.Ageable) object).setAge(age);
	}
}

@Override
@Nullable
public Class<?>[] acceptChange(ChangeMode mode) {
	if (isMax || mode == ChangeMode.REMOVE_ALL)
		return null;
	return CollectionUtils.array(Number.class);
}

@Override
public void change(Event event, @Nullable Object[] delta, ChangeMode mode) {
	int value = 0;
	switch (mode) {
		case ADD:
			if (delta == null || delta[0] == null)
				return;
			value = ((Number)delta[0]).intValue();
			for (Object object : getExpr().getArray(event)) {
				int existing = convert(object);
				setAge(object, existing + value);
			}
			break;
		case REMOVE:
			if (delta == null || delta[0] == null)
				return;
			value = ((Number)delta[0]).intValue();
			for (Object object : getExpr().getArray(event)) {
				int existing = convert(object);
				setAge(object, existing - value);
			}
			break;
		case SET:
			if (delta == null || delta[0] == null)
				return;
			value = ((Number)delta[0]).intValue();
			// Fall through as the value will be 0 for others.
		case DELETE:
		case RESET:
			for (Object object : getExpr().getArray(event))
				setAge(object, value);
			break;
		default:
			break;
	}
}

@AyhamAl-Ali AyhamAl-Ali marked this pull request as draft June 30, 2022 17:38
@AyhamAl-Ali AyhamAl-Ali marked this pull request as ready for review June 30, 2022 19:30
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.

Ensure that you're returning the actual Number type like Integer as the generic instead of Number. The acceptChange is still fine with Number.

@AyhamAl-Ali AyhamAl-Ali requested a review from TheLimeGlass June 30, 2022 21:00
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.

When you make your commit messages, try to be more descriptive rather than just "RC" for requested changes.

Also if you want, we could add a test case to this as it's an object type expression and does runtime checking, but optional as we have many other tests similar to this.

@TheLimeGlass
Copy link
Collaborator

Targeting 2.6.3

@TheLimeGlass
Copy link
Collaborator

TheLimeGlass commented Jul 1, 2022

Merged master to fix actions from being hung

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.

Just the one thing and then an idea for discussion

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.

Should be good

@TheLimeGlass TheLimeGlass added 2.7 Targeting a 2.7.X version release 2.6 labels Aug 6, 2022
@TheLimeGlass TheLimeGlass removed the 2.6 label Aug 6, 2022
@TheLimeGlass TheLimeGlass merged commit 89f7926 into SkriptLang:master Aug 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.7 Targeting a 2.7.X version release bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants