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

Fix/number converters #5079

Merged
merged 4 commits into from
Jan 28, 2023
Merged

Conversation

TPGamesNL
Copy link
Member

Description

Adds more number converters, from the generic java.lang.Number type to specific subclasses.


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

@TPGamesNL TPGamesNL added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Sep 2, 2022
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 suggest adding a test for every new Number converter. This may also cause issues with existing syntaxes due to the nature of using Number as the return type rather than it's actual type like Hunger of a player returning Double. Most syntaxes just return Number (Mainly addons, but Skript still has some old syntaxes that do this)

@TPGamesNL
Copy link
Member Author

@TheLimeGlass How would you imagine writing such tests in a script? The only types scripts can use are number (java.lang.Number) and integer (java.lang.Long), and no syntaxes require a specific number type, all take %number%. This requirement would no longer be needed after this PR, as converters would take care of it. However, it would also not be worth changing, unless the syntax element requires the wrapper type instead of the primitive, which is not often.

I also don't see how it could cause issues, can you elaborate on that?

@TheLimeGlass
Copy link
Collaborator

@TheLimeGlass How would you imagine writing such tests in a script? The only types scripts can use are number (java.lang.Number) and integer (java.lang.Long), and no syntaxes require a specific number type, all take %number%. This requirement would no longer be needed after this PR, as converters would take care of it. However, it would also not be worth changing, unless the syntax element requires the wrapper type instead of the primitive, which is not often.

I also don't see how it could cause issues, can you elaborate on that?

As discussed, JUnit tests are gonna be best for this solution.

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.

@TPGamesNL conflicts

@TheLimeGlass TheLimeGlass added the 2.7 Targeting a 2.7.X version release label Jan 27, 2023
@TheLimeGlass TheLimeGlass merged commit f7eb3d2 into SkriptLang:master Jan 28, 2023
@TPGamesNL TPGamesNL deleted the fix/number-converters branch January 28, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.7 Targeting a 2.7.X version release 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