Skip to content

Conversation

DerEchtePilz
Copy link
Member

@DerEchtePilz DerEchtePilz commented Apr 6, 2024

This PR is supposed to implement certain methods as suggested by Hawk here and by #535.

Currently, there is a bit more testing I want to do, additionally I am not sure if this would actually be something we want.
If so, there is a bit more work required:

@DerEchtePilz

This comment was marked as outdated.

@DerEchtePilz DerEchtePilz requested a review from JorelAli April 9, 2024 20:28
@DerEchtePilz DerEchtePilz linked an issue Apr 11, 2024 that may be closed by this pull request
@DerEchtePilz DerEchtePilz force-pushed the dev/safe-arguments branch 2 times, most recently from 7afb060 to 4aca81f Compare April 12, 2024 19:51
@willkroboth
Copy link
Collaborator

Just a note that some arguments use a raw type as their T generic parameter. For example, MapArgument<K, V> extends Argument<LinkedHashMap> rather than Argument<LinkedHashMap<K, V>>

https://github.com/JorelAli/CommandAPI/blob/a7d2717450c017002647fe611a3d39deda6874d2/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/arguments/MapArgument.java#L26

If MapArgument did extend Argument<LinkedHashMap<K, V>>, then the getPrimitiveType method would have to return a Class<LinkedHashMap<K, V>> object, which Java doesn't like very much: https://stackoverflow.com/questions/1079279/class-object-of-generic-class-java.

That just means this code has a little warning:

MapArgument<String, Integer> argument = new MapArgumentBuilder<String, Integer>("map")
	.withKeyMapper(s -> s)
	.withValueMapper(s -> Integer.parseInt(s))
	.withoutKeyList()
	.withoutValueList()
	.build();

new CommandAPICommand("command")
	.withArguments(argument)
	.executes(info -> {
		// Type safety: The expression of type LinkedHashMap needs unchecked conversion to conform to Map<String,Integer>
		Map<String, Integer> map = info.args().getByArgument(argument);
		info.sender().sendMessage(map.toString());
	})
	.register();

This note might also be relevant for #545. It's not a big problem, but the two systems don't currently work perfectly for these arguments:

@DerEchtePilz
Copy link
Member Author

True, but if I am not mistaken, this is the best we can do here.
Since it additionally doesn't work for the CustomArgument anyway, I guess in terms of safety we can ignore the generic types.

@DerEchtePilz DerEchtePilz force-pushed the dev/safe-arguments branch 2 times, most recently from 656cb5c to 9c6af63 Compare April 23, 2024 18:56
Copy link
Member

@JorelAli JorelAli left a comment

Choose a reason for hiding this comment

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

A couple of minor documentation changes and code linting tweaks (avoid the use of variables which declared non-final and are only ever used once). A critical change required for PRIMITIVE_TO_WRAPPER

@DerEchtePilz DerEchtePilz force-pushed the dev/safe-arguments branch 2 times, most recently from b8c182e to 8315291 Compare April 24, 2024 06:41
@DerEchtePilz DerEchtePilz merged commit 07bc9c9 into dev/dev Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cast object to class like as it is done in JOOQ
4 participants