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

Cleanup EnumUtils #5744

Merged
merged 6 commits into from
Jan 1, 2024

Conversation

APickledWalrus
Copy link
Member

Description

This PR updates EnumUtils with better method names and javadoc. I've also addressed the issue reported in #5743 by cutting down on the validation performed. In its current state, it's rather excessive as it tries to handle runtime changes to enum changes via reflection. I likely broke this during my PR to add gender support to this class. However, I have opted to remove these checks completely as I don't feel it's necessary for us to support it in any way. Modifying enums during runtime is risky and unlikely to take place anyways. If you think we should handle this, I am happy to discuss it.

I have also opted to change the "Missing entry" from a warning to a debug message (similar to the "generating temporary alias" message). This message does not need to be displayed for he average user. Also, it can repeat multiple times if the language change listeners activate, like when another addon loads a language (see screenshot below).

image


Target Minecraft Versions:
Requirements:
Related Issues:

This commit removes some validation performed by the class. Notably, validation will only be performed when the language updates, meaning this class no longer supports runtime Enum changes.
@APickledWalrus APickledWalrus added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Jun 19, 2023
@APickledWalrus APickledWalrus changed the base branch from master to dev/feature September 16, 2023 18:07
Copy link
Collaborator

@TheLimeGlass TheLimeGlass left a comment

Choose a reason for hiding this comment

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

I think this should target dev/patch

Copy link
Contributor

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

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

Just mild things, only one of them actually matters, forgot to make it requesting changes

src/main/java/ch/njol/skript/util/EnumUtils.java Outdated Show resolved Hide resolved
@APickledWalrus APickledWalrus added the 2.8 Targeting a 2.8.X version release label Nov 20, 2023
@sovdeeth sovdeeth added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Dec 31, 2023
@sovdeeth sovdeeth merged commit 7b669c9 into SkriptLang:dev/feature Jan 1, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 Targeting a 2.8.X version release enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants