-
-
Notifications
You must be signed in to change notification settings - Fork 373
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
Ban Entry Syntaxes #7257
base: dev/feature
Are you sure you want to change the base?
Ban Entry Syntaxes #7257
Conversation
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.
just stuff i noticed
private Expression<Object> banTargetExpr; // offline player or string (ip) | ||
|
||
@Override | ||
public boolean init(Expression<?>[] expressions, int matchedPattern, Kleenean isDelayed, SkriptParser.ParseResult parseResult) { |
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.
public boolean init(Expression<?>[] expressions, int matchedPattern, Kleenean isDelayed, SkriptParser.ParseResult parseResult) { | |
public boolean init(Expression<?>[] expressions, int matchedPattern, Kleenean isDelayed, ParseResult parseResult) { |
} | ||
|
||
@Override | ||
public Class<?> @Nullable [] acceptChange(Changer.ChangeMode mode) { |
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.
public Class<?> @Nullable [] acceptChange(Changer.ChangeMode mode) { | |
public Class<?> @Nullable [] acceptChange(ChangeMode mode) { |
@Description("Returns data about a player or IP ban. " + | ||
"The source can be any string, but it's usually the player's name. " + | ||
"The expiration date won't return a value if the ban is permanent.") | ||
@Examples({"set {_player} to offlineplayer(\"Notch\", false)", |
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.
@Examples({"set {_player} to offlineplayer(\"Notch\", false)", | |
@Examples({ | |
"set {_player} to offlineplayer(\"Notch\", false)", |
"set {_expiration} to the date {_player}'s ban expires", | ||
"set {_time.left} to difference between now and {_expiration}", | ||
"set {_reason} to the reason {_player} was banned", | ||
"send \"There is %{_time.left}% before %{_player}% gets unbanned! They were banned for '%{_reason}%'\" to 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.
"send \"There is %{_time.left}% before %{_player}% gets unbanned! They were banned for '%{_reason}%'\" to player"}) | |
"send \"There is %{_time.left}% before %{_player}% gets unbanned! They were banned for '%{_reason}%'\" to player" | |
}) |
if (Skript.methodExists(BanEntry.class, "remove")) | ||
Skript.registerExpression(ExprBanData.class, Object.class, ExpressionType.SIMPLE, | ||
"[the] date %offlineplayer/string% was banned", | ||
"[the] date of %offlineplayer/string%'s ban", |
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.
%offlineplayer/string%'[s] ban date
?
} | ||
|
||
@Override | ||
public void change(Event event, Object @Nullable [] delta, Changer.ChangeMode mode) { |
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.
public void change(Event event, Object @Nullable [] delta, Changer.ChangeMode mode) { | |
public void change(Event event, Object @Nullable [] delta, ChangeMode mode) { |
static { | ||
if (Skript.methodExists(BanEntry.class, "remove")) | ||
Skript.registerExpression(ExprBanData.class, Object.class, ExpressionType.SIMPLE, | ||
"[the] date %offlineplayer/string% was banned", |
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.
Syntax is not a good option, it can be very confusing for readers and might affect the parser. I suggest something like ban date of ...
as Efnilite also suggested.
Same goes for the rest of syntaxes, ban X of Y
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.
This current one seems much better than ban x of y to me.
Is there a reason you think it's poor?
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.
date "SkriptDev" was banned
seems off to me at first glance, it doesn't immediately gives you a hint that it's a ban date. IMO ban date
is easier to quickly recognize it's a ban date.
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'd be improved by adding that
:
the date that "SkriptDev" was banned
It buries the lead by like 2 words which i don't really think is an issue, especially when it's so much more natural/fluid imo. ban date of "SkriptDev"
feels much more stilted and awkward and something i wouldn't actually say irl.
I'm not opposed to having it in addition, but i definitely think the current syntax is higher quality
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.
so do i add another syntax for ban date
or ..?
Co-authored-by: Efnilite <[email protected]>
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.
Support for multiple offlineplayers or strings?
Description
This PR lets you get details about player/IP bans, such as the date the ban was issued, who banned them, the expiration date, and the reason for the ban.
You can set/add/remove from the expiration date and set the source/reason. You can technically set the ban creation date but I didn't include it in the PR.
Examples
Target Minecraft Versions: 1.20.1+
Requirements: none
Related Issues: none