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

Rework number parser and angle support #7139

Open
wants to merge 39 commits into
base: dev/feature
Choose a base branch
from

Conversation

Efnilite
Copy link
Member

@Efnilite Efnilite commented Oct 9, 2024

Description

Reworks number parser. Adds support for angles. When using x degrees, it is just syntactic sugar for x. When using x radians, converts x to degrees.


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

@Efnilite Efnilite added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Oct 9, 2024
@Efnilite Efnilite requested a review from sovdeeth October 9, 2024 14:14
Copy link
Member

@UnderscoreTud UnderscoreTud left a comment

Choose a reason for hiding this comment

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

This should be baked into the number parser. That way it can be used in contexts where only literals are accepted

@Efnilite Efnilite requested a review from UnderscoreTud October 9, 2024 17:03
@Efnilite Efnilite changed the title Add angle expression Rework number parser and angle support Oct 10, 2024
Copy link
Member

@cheeezburga cheeezburga left a comment

Choose a reason for hiding this comment

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

Only had a quick look through but looks good! The only thing I'd say is that the test could maybe cover some edge cases too (e.g. NaN, infinity, invalid values like strings, etc.)?

@Efnilite Efnilite requested a review from sovdeeth October 12, 2024 12:56
@Efnilite Efnilite requested a review from sovdeeth October 13, 2024 14:15
# Conflicts:
#	src/main/java/ch/njol/skript/classes/data/JavaClasses.java
#	src/main/java/ch/njol/skript/effects/EffVectorRotateAroundAnother.java
#	src/main/java/ch/njol/skript/effects/EffVectorRotateXYZ.java
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

tests failing :(

Comment on lines +32 to +36
Skript.registerExpression(ExprAngle.class, Number.class, ExpressionType.SIMPLE,
"%number% [in] deg[ree][s]",
"%number% [in] rad[ian][s]",
"%numbers% in deg[ree][s]",
"%numbers% in rad[ian][s]");
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to support gradians (grad[s]/gon) while you're here?
They're pretty obscure nowadays so I don't mind, but you do occasionally see them used in map stuff and cartography.

@Moderocky
Copy link
Member

Good PR. I wondered about supporting the symbols too (e.g. 90°) but they're not very standardised so it's not essential.

@Efnilite Efnilite added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants