-
-
Notifications
You must be signed in to change notification settings - Fork 351
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
EffectCommandEvent: SkriptEvent support #6575
EffectCommandEvent: SkriptEvent support #6575
Conversation
# Conflicts: # src/main/java/ch/njol/skript/classes/data/BukkitEventValues.java
private enum Executor { | ||
|
||
ANY(""), | ||
CONSOLE("console "), | ||
PLAYER("player "); | ||
|
||
final String toString; | ||
|
||
Executor(String toString) { | ||
this.toString = toString; | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return toString; | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's probably best to ignore an enum use Kleenean#get(int)
so something like [-1:player|1:console]
TRUE = console, FALSE = player and UNKNOWN = any. it's a bit weirder but removes unneeded enums. From here it's basically Kleenean.get(parseResult.mark)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see this implementation will definitely put into a good use!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vastly prefer an Enum, a Kleenean doesn't make much contextual sense to me. That said, you could simplify the logic a bit by using parse marks and the ordinal of the enum, if you wanted.
private enum Executor { | ||
|
||
ANY(""), | ||
CONSOLE("console "), | ||
PLAYER("player "); | ||
|
||
final String toString; | ||
|
||
Executor(String toString) { | ||
this.toString = toString; | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return toString; | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vastly prefer an Enum, a Kleenean doesn't make much contextual sense to me. That said, you could simplify the logic a bit by using parse marks and the ordinal of the enum, if you wanted.
} else if (event instanceof ScriptCommandEvent) { | ||
ScriptCommandEvent e = (ScriptCommandEvent) event; | ||
command = e.getCommandLabel() + " " + e.getArgsString(); | ||
} else { // It's an EffectCommandEvent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
best to make sure
"on effect command:", | ||
"\tlog \"%sender%: %command%\" to file \"effectcommand.log\"" | ||
}) | ||
@Since("2.0, 2.7 (support for script commands), INSERT VERSION (support for effect command)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Since("2.0, 2.7 (support for script commands), INSERT VERSION (support for effect command)") | |
@Since("2.0, 2.7 (script commands), INSERT VERSION (effect commands)") |
"\t\t\tcancel the event", | ||
"on effect command:", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"\t\t\tcancel the event", | |
"on effect command:", | |
"\t\t\tcancel the event", | |
"", | |
"on effect command:", |
CONSOLE("console "), | ||
PLAYER("player "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONSOLE("console "), | |
PLAYER("player "); | |
CONSOLE("console"), | |
PLAYER("player"); |
spacing should be handled by the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of instead handling the spacing in the toString(), why not do it in the enum to string instead. Whoops
.description("Called when a player or console executes a skript effect command.") | ||
.examples( | ||
"on effect command:", | ||
"\tlog \"%sender%: %command%\" to file \"effectcommand.log\"") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"\tlog \"%sender%: %command%\" to file \"effectcommand.log\"") | |
"\tlog \"%sender%: %command%\" to file \"effectcommands.log\"") |
Closing, in favour of #6587 |
Description
This PR adds support for Skript users to listen to EffectCommandEvent, and cleans up the involved classes.
Event values are:
event-sender
returns the sender (i.e. Player, Console)[the] command
(from ExprCommand) returns the involved commandTarget Minecraft Versions: any
Requirements: none
Related Issues: none