Skript icon indicating copy to clipboard operation
Skript copied to clipboard

Add a getter method for converting in Expression class

Open TheLimeGlass opened this issue 3 years ago • 5 comments

Description

Similar to the Expression#check method, this method will get all the values and then use the provided converter to convert the values to the requested output. This is super handy for everyone, PropertyExpression currently has an almost exact method, so why not on Expression too to make things so much easier.

Example of how this method can be utilized:

  • https://github.com/TheLimeGlass/Skript/blob/item-amount-type/src/main/java/ch/njol/skript/expressions/ExprItemAmount.java
  • https://github.com/SkriptLang/Skript/pull/4632#discussion_r928048655

This link above is a future pull request for when this gets merged to add support for itemtype checking in the item amount expression, related suggest #2142

TheLimeGlass avatar Jul 12 '22 08:07 TheLimeGlass

The method should probably also be named getArray (or mb getConvertedArray) so reflect which method it uses internally.

TPGamesNL avatar Jul 12 '22 08:07 TPGamesNL

The method should probably also be named getArray (or mb getConvertedArray) so reflect which method it uses internally.

I was thinking about that, I was also thinking convert, but convert is already a method in SimplePropertyExpression and there is already Expression#getConverteredExpression I wanted to make it closer to the exact method it mimics which is PropertyExpression#get(Object[], Converter)

TheLimeGlass avatar Jul 12 '22 09:07 TheLimeGlass

I think because Expression has multiple get methods (such as getArray and getAll, having different behaviour), it's better to be explicit in the naming which to use. Therefore, I think getArray (same name as regular method, just with a converter param) or getConvertedArray (still implying getArray, but also more explicitly indicating it has a converter) would be better

TPGamesNL avatar Jul 12 '22 10:07 TPGamesNL

Needs reviews, this helps tremendously, makes code shorter and easier to read.

TheLimeGlass avatar Aug 03 '22 10:08 TheLimeGlass

I'm not sure I see the value here. Why not expr.stream(e).map(converter::convert)?

Pikachu920 avatar Sep 21 '22 03:09 Pikachu920

I'm not sure I see the value here. Why not expr.stream(e).map(converter::convert)?

Way shorter, direct usage to lambdas, and nested expressions. Streams you also have to convert to array, stream, and map.

Demonstrated difference in link mentioned https://github.com/SkriptLang/Skript/pull/4632#discussion_r928048655

TheLimeGlass avatar Oct 06 '22 10:10 TheLimeGlass

I'm not sure I see the value here. Why not expr.stream(e).map(converter::convert)?

Way shorter, direct usage to lambdas, and nested expressions. Streams you also have to convert to array, stream, and map.

Demonstrated difference in link mentioned #4632 (comment)

Coping your example here for ease of viewing: image

While I would agree that your second example looks cleaner, I feel like the use cases requiring the use of flatMap are somewhat rare. In addition, neither of those examples are ideal as they both call a getter method of the types expression multiple times.

Pikachu920 avatar Oct 06 '22 23:10 Pikachu920

This is still very much valid. Coming back because this still comes up with modern syntaxes.

TODO add stream collect method

@Override
protected PotionEffect[] get(Event event, LivingEntity[] source) {
	return Arrays.stream(source)
			.flatMap(entity -> entity.getActivePotionEffects().stream())
			.toArray(PotionEffect[]::new);
}

vs a proper collect method

@Override
protected PotionEffect[] get(Event event, LivingEntity[] source) {
	return collect(source, entity -> entity.getActivePotionEffects().stream());
	// and another method for collections.
	return collect(source, entity -> entity.getActivePotionEffects());
}

TheLimeGlass avatar Jul 01 '23 05:07 TheLimeGlass