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 warnings for using %player% without using the UUID config setting. #6271

Merged
merged 4 commits into from
Jan 1, 2024

Conversation

sovdeeth
Copy link
Member

@sovdeeth sovdeeth commented Dec 31, 2023

Description

As a precursor to #5676, we need to add warnings to users to warn them of the incoming changes. This PR adds a warning whenever an expression with a Player return type is used in a VariableString, if the use UUIDs in variable names config option is false.

It directs users to #6270 to learn more about how to address the warning.

Ideally, this prompts players to switch to UUIDs through the config, but forcing them to use player's name is also an acceptable outcome.

Critique on the warning's content and the explanatory discussion post is welcome.

image


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

@sovdeeth sovdeeth added enhancement Feature request, an issue about something that could be improved, or a PR improving something. 2.8 Targeting a 2.8.X version release labels Dec 31, 2023
@Fusezion
Copy link
Contributor

Is this on local variables and anyway to disable it without swapping to uuid? I have global temp vars I don't need a uuid for

@BaeFell
Copy link
Member

BaeFell commented Dec 31, 2023

Is this on local variables and anyway to disable it without swapping to uuid? I have global temp vars I don't need a uuid for

Doesn't changing the config option also change it in local variables?

@sovdeeth
Copy link
Member Author

Is this on local variables and anyway to disable it without swapping to uuid? I have global temp vars I don't need a uuid for

Yes, this does apply to local variables too. We could disable the warning for those, but you really should be using player's name if you intend to use the player's name specifically.

@BaeFell
Copy link
Member

BaeFell commented Dec 31, 2023

Should there be a config option to disable this warning? Similar to the variable conflict warning?

@Fusezion
Copy link
Contributor

I'll agree on using players name but I also prefer being lazy

@sovdeeth
Copy link
Member Author

Should there be a config option to disable this warning? Similar to the variable conflict warning?

I considered that, but I fear users would just disable the warning and get slapped in the face when we do change it.

@BaeFell
Copy link
Member

BaeFell commented Dec 31, 2023

Should there be a config option to disable this warning? Similar to the variable conflict warning?

I considered that, but I fear users would just disable the warning and get slapped in the face when we do change it.

Fair point. It's not gonna be forever to be fair, so makes sense not to have an option to turn it off.

Copy link
Member

@BaeFell BaeFell left a comment

Choose a reason for hiding this comment

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

I like this error message and the linked article about migrating.

@AyhamAl-Ali
Copy link
Member

I read the discussion and all good but I wonder about offlineplayer usage in variables as well, is it also affected by the change? if so the warning should account for that as well

@sovdeeth
Copy link
Member Author

I read the discussion and all good but I wonder about offlineplayer usage in variables as well, is it also affected by the change? if so the warning should account for that as well

Good catch, thanks! They're affected in the exact same way as Players, so I'll include them in the warning.

@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 5bc25d6 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.

5 participants