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

feat: Added the option to use all the stats in the conditions #357

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

dm94
Copy link
Contributor

@dm94 dm94 commented May 9, 2024

No description provided.

@dm94 dm94 changed the title feat: Added the option to use all the stats feat: Added the option to use all the stats in the conditions May 9, 2024
Comment on lines 39 to 52
for (Stats.General statGeneral : Stats.General.values()) {
if (statGeneral.name().equalsIgnoreCase(key))
return statGeneral;
}

for (Stats.Bot statBot : Stats.Bot.values()) {
if (statBot.name().equalsIgnoreCase(key))
return statBot;
}

for (Stats.BootyKey statKey : Stats.BootyKey.values()) {
if (statKey.name().equalsIgnoreCase(key))
return statKey;
}
Copy link
Member

Choose a reason for hiding this comment

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

instead of hard-coding the bot's stats, you should be using the stat API to grab any of the registered stats. The point here is that 3rd party plugins can register their own stats and with a custom tracker update the values, and they should be possible to use here too!

Consider having a syntax for namespaced keys, so you can have:
experience: namespace -> null (the bot's namespace), category -> any, name -> experience
dmplugin:experience: namespace -> dmplugin, category -> any, name -> experience
dmplugin:general:experience: namespace -> dmplugin, category -> general, name -> experience

To implement this matching logic you can probably expose in StatsManager and StatsAPI a:

public Collection<? extends StatsAPI.Key> getStatKeys() {
  return Collections.unmodifiableSet(statistics.keySet());
}

and use that to search for the category for the namespace/key given, if none is found defer the operation to later as the stat could've not been registered yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implementation done, I'm not very proud of this implementation but I can't think of any other way right now.

Copy link
Member

@Pablete1234 Pablete1234 left a comment

Choose a reason for hiding this comment

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

Misclicked previous review as approved, this needs the changes mentioned

}
return null;
private StatsAPI.Key getKeyFromString(String token) {
String[] tokenParts = token.split(":");
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
String[] tokenParts = token.split(":");
String[] tokenParts = token.split(":", 3);

Copy link
Member

Choose a reason for hiding this comment

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

If you don't cap it at 3, you may have a string like:
plugin:something:general:uridium
which would be parsed as
namespace=plugin, category=general, name=uridium, leaving the "something" being completely ignored. This is faulty behavior, so you either limit it to 3 splits or you throw an error if there's more than 3, but it needs to be handled

@dm94
Copy link
Contributor Author

dm94 commented Jun 23, 2024

I think the way it is now is the most correct way as it allows backwards compatibility to be maintained.

@dm94 dm94 requested a review from Pablete1234 June 23, 2024 08:33
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.

2 participants