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

Fixes the warnings in the default variable test #5606

Merged
merged 3 commits into from
Apr 7, 2024

Conversation

TheLimeGlass
Copy link
Collaborator

@TheLimeGlass TheLimeGlass commented Apr 15, 2023

Description

Fixes the warnings in the default variable test.
Warnings that this pull request removes:

[21:52:11 INFO]: [Skript] Line 6: (..\..\..\..\..\..\src\test\skript\tests\misc\default variables.sk)
[21:52:11 INFO]:     It is suggested to use brackets around the name of a variable. Example: {example::%player%} = 5
[21:52:11 INFO]: Excluding brackets is deprecated, meaning this warning will become an error in the future.
[21:52:11 INFO]:     Line: testing::no brackets::%living entity%: "String"
[21:52:11 INFO]:
[21:52:11 INFO]: [Skript] Line 7: (..\..\..\..\..\..\src\test\skript\tests\misc\default variables.sk)
[21:52:11 INFO]:     It is suggested to use brackets around the name of a variable. Example: {example::%player%} = 5
[21:52:11 INFO]: Excluding brackets is deprecated, meaning this warning will become an error in the future.
[21:52:11 INFO]:     Line: testing::variables4: 1.6900000000001
[21:52:11 INFO]:

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

@TheLimeGlass TheLimeGlass added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Apr 15, 2023
@AyhamAl-Ali
Copy link
Member

Why tho? Isn't the goal of the code you changed is to test the warning feature? it even says no brackets as it's intentional

I would suggest keeping just line 6 and rename the variable to like no brackets warning test but keep the change of line 7

@TheLimeGlass
Copy link
Collaborator Author

TheLimeGlass commented Apr 15, 2023

Why tho? Isn't the goal of the code you changed is to test the warning feature? it even says no brackets as it's intentional

I would suggest keeping just line 6 and rename the variable to like no brackets warning test but keep the change of line 7

I added it because when I developed the default variables I needed to make sure that no brackets worked. Then the warning came along where no brackets gets considered a StringMode.MESSAGE so it's encouraged to make sure it's a variable now (StringMode.VARIABLE...). The warnings are annoying now when in development mode.

@Moderocky Moderocky force-pushed the master branch 2 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59
@Moderocky Moderocky changed the base branch from master to dev/feature September 18, 2023 10:36
@sovdeeth
Copy link
Member

Why tho? Isn't the goal of the code you changed is to test the warning feature? it even says no brackets as it's intentional
I would suggest keeping just line 6 and rename the variable to like no brackets warning test but keep the change of line 7

I added it because when I developed the default variables I needed to make sure that no brackets worked. Then the warning came along where no brackets gets considered a StringMode.MESSAGE so it's encouraged to make sure it's a variable now (StringMode.VARIABLE...). The warnings are annoying now when in development mode.

Surely it's still valuable to test that it works, though?

@TheLimeGlass
Copy link
Collaborator Author

Why tho? Isn't the goal of the code you changed is to test the warning feature? it even says no brackets as it's intentional
I would suggest keeping just line 6 and rename the variable to like no brackets warning test but keep the change of line 7

I added it because when I developed the default variables I needed to make sure that no brackets worked. Then the warning came along where no brackets gets considered a StringMode.MESSAGE so it's encouraged to make sure it's a variable now (StringMode.VARIABLE...). The warnings are annoying now when in development mode.

Surely it's still valuable to test that it works, though?

Well then we need Pikachu's pull request that adds a test runner feature that acts like a try and catch. It's still pending.

@sovdeeth
Copy link
Member

sovdeeth commented Feb 1, 2024

I think this change should be done when we do deprecate the brackets. Until then it's still valuable to test.

@TheLimeGlass
Copy link
Collaborator Author

TheLimeGlass commented Feb 6, 2024

I think this change should be done when we do deprecate the brackets. Until then it's still valuable to test.

They are deprecated. Do you mean remove? Why shouldn't this be merged now? It's just for testing sake? Tired of seeing this error like it's a new error that a branch I made did, when it's not.

@sovdeeth
Copy link
Member

sovdeeth commented Feb 6, 2024

I think this change should be done when we do deprecate the brackets. Until then it's still valuable to test.

They are deprecated. Do you mean remove? Why shouldn't this be merged now? It's just for testing sake? Tired of seeing this error like it's a new error that a branch I made did, when it's not.

yeah, I meant remove. I suppose since it's targeting feature it's fine to merge. I just wanted to make sure we're still testing things that are intended to work in 2.8.

@TheLimeGlass
Copy link
Collaborator Author

I think this change should be done when we do deprecate the brackets. Until then it's still valuable to test.

They are deprecated. Do you mean remove? Why shouldn't this be merged now? It's just for testing sake? Tired of seeing this error like it's a new error that a branch I made did, when it's not.

yeah, I meant remove. I suppose since it's targeting feature it's fine to merge. I just wanted to make sure we're still testing things that are intended to work in 2.8.

I fixed default variables in 2.7. It is unrelated to 2.8 and can be included in 2.8.

@Moderocky Moderocky merged commit 0827e01 into dev/feature Apr 7, 2024
6 checks passed
@Moderocky Moderocky deleted the fix/default-variables-warning-test branch April 7, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants