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

Add playerexpcooldownchange event #5822

Closed
wants to merge 12 commits into from
Closed

Add playerexpcooldownchange event #5822

wants to merge 12 commits into from

Conversation

nylhus
Copy link
Contributor

@nylhus nylhus commented Jul 12, 2023

Description

Added PlayerExpCooldownEvent.

Target Minecraft Versions:
Requirements:
Related Issues: #5802

Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

nyhlus pr!!

src/main/java/ch/njol/skript/events/SimpleEvents.java Outdated Show resolved Hide resolved
import javax.annotation.Nullable;

@Name("Exp Cooldown")
@Description("The exp cooldown of a player")
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate more? What's an exp cooldown? Is it a cooldown for gaining experience, is it a cooldown for picking up experience, for dropping experience, etc.

@Override
@Nullable
public Class<?>[] acceptChange(ChangeMode mode) {
return (mode != ChangeMode.REMOVE_ALL) ? CollectionUtils.array(Timespan.class) : null;
Copy link
Member

Choose a reason for hiding this comment

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

May want to default to null if given an unexpected new ChangeMode, rather than accepting it. A switch is often recommended, I believe.

nylhus and others added 3 commits July 12, 2023 13:34
@AyhamAl-Ali AyhamAl-Ali added the feature Pull request adding a new feature. label Jul 12, 2023

import javax.annotation.Nullable;

@Name("Exp Cooldown")
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to not use the abbreviated name here.

public Class<? extends Timespan> getReturnType() { return Timespan.class; }

@Override
protected String getPropertyName() { return "exp cooldown"; }
Copy link
Member

Choose a reason for hiding this comment

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

The property should probably be experience cooldown.


static {
if (Skript.methodExists(Player.class, "getExpCooldown"))
register(ExprExpCooldown.class, Timespan.class, "exp[erience] [pickup] cooldown change", "players");
Copy link
Member

Choose a reason for hiding this comment

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

It looks like change is a necessary element in the pattern but I thought that was for the event, not the property expression.

src/main/java/ch/njol/skript/events/SimpleEvents.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/SimpleEvents.java Outdated Show resolved Hide resolved
import org.bukkit.entity.Player;
import org.bukkit.event.Event;

import javax.annotation.Nullable;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use Jetbrains or Eclipse Nullable, we don't typically use javax

@Override
@Nullable
public Class<?>[] acceptChange(ChangeMode mode) {
return (mode != ChangeMode.REMOVE_ALL) ? CollectionUtils.array(Timespan.class) : null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Skript doesn't do one liners, changes need similar to the convert

nylhus and others added 6 commits July 14, 2023 13:37
Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
@Moderocky Moderocky force-pushed the master branch 3 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59
@sovdeeth sovdeeth changed the base branch from master to dev/feature December 30, 2023 07:29
@sovdeeth
Copy link
Member

sovdeeth commented Apr 7, 2024

Closing due to 9+ months of inactivity.

@sovdeeth sovdeeth closed this Apr 7, 2024
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.

5 participants