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

EffectCommandEvent: SkriptEvent support #6575

Conversation

NotSoDelayed
Copy link
Contributor

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 command

Target Minecraft Versions: any
Requirements: none
Related Issues: none

Comment on lines +44 to +61
private enum Executor {

ANY(""),
CONSOLE("console "),
PLAYER("player ");

final String toString;

Executor(String toString) {
this.toString = toString;
}

@Override
public String toString() {
return toString;
}

}
Copy link
Contributor

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)

Copy link
Contributor Author

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!

Copy link
Member

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.

Comment on lines +44 to +61
private enum Executor {

ANY(""),
CONSOLE("console "),
PLAYER("player ");

final String toString;

Executor(String toString) {
this.toString = toString;
}

@Override
public String toString() {
return toString;
}

}
Copy link
Member

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
Copy link
Member

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)")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@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)")

Comment on lines +48 to +49
"\t\t\tcancel the event",
"on effect command:",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"\t\t\tcancel the event",
"on effect command:",
"\t\t\tcancel the event",
"",
"on effect command:",

Comment on lines +47 to +48
CONSOLE("console "),
PLAYER("player ");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CONSOLE("console "),
PLAYER("player ");
CONSOLE("console"),
PLAYER("player");

spacing should be handled by the caller.

Copy link
Contributor Author

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

@AyhamAl-Ali AyhamAl-Ali added the feature Pull request adding a new feature. label Apr 16, 2024
.description("Called when a player or console executes a skript effect command.")
.examples(
"on effect command:",
"\tlog \"%sender%: %command%\" to file \"effectcommand.log\"")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"\tlog \"%sender%: %command%\" to file \"effectcommand.log\"")
"\tlog \"%sender%: %command%\" to file \"effectcommands.log\"")

@NotSoDelayed NotSoDelayed marked this pull request as draft April 19, 2024 05:00
NotSoDelayed added a commit to NotSoDelayed/Skript that referenced this pull request Apr 19, 2024
@NotSoDelayed
Copy link
Contributor Author

Closing, in favour of #6587

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants