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 math order #3736

Merged
merged 7 commits into from
May 3, 2021
Merged

Fix math order #3736

merged 7 commits into from
May 3, 2021

Conversation

TPGamesNL
Copy link
Member

Description

Fixes Skript's order of mathematical operations.

Each pattern now has all permutations of surrounding parentheses on the %number%s, because this information would otherwise be lost in the parsing process. The patterns with parentheses should come first, otherwise it wouldn't have effect, as Skript would match it against the other patterns.

The expression forms a chain of alternating expression and operators on init (e.g. [Literal(5), Operator(PLUS), Literal(3)]). The first part of the chain is the first parameter, the second is the operator and the third is the second parameter. If either of the two parameters is an instance of ExprArithmetic (and is not surrounded by parentheses), it is not added to the chain, but instead parameters chain is added. This chain is then parsed using the proper operation order, and this parsed chain (forming a tree structure) is used for evaluating the expression.

Two things I'd appreciate feedback on:

  • whether all of these inner classes wouldn't be better of as normal classes
  • whether the chain parsing should be done on init, or in the first evaluation (since doing it in init is useless for any inner ExprArithmetics, which won't be if it's called in get, with the tradeoff of having to parse it on runtime)

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

@WeeskyBDW
Copy link

Imo it will be better to split arthimetic pure logic and expression. Also, dont forget to comment methods who aren't explicits (if i remember, i'll submit changes where i think comment or Javadoc is necessary).

@TPGamesNL
Copy link
Member Author

Imo it will be better to split arthimetic pure logic and expression.

I don't understand what you mean by this, can you explain it?

@WeeskyBDW
Copy link

Imo it will be better to split arthimetic pure logic and expression.

I don't understand what you mean by this, can you explain it?

as possible, split more methods/enums in a other file (it's my personal opinion, it can be false)

@TPGamesNL
Copy link
Member Author

I agree, but that would probably mean placing it in ch.njol.skript.expressions.arithmetic or something alike, and I'm not sure if the devs want a package there for a single expression (would like to hear some input from you guys, as always).

@WeeskyBDW
Copy link

I agree, but that would probably mean placing it in ch.njol.skript.expressions.arithmetic or something alike, and I'm not sure if the devs want a package there for a single expression (would like to hear some input from you guys, as always).

imo, it can be do for arithmetic expression because it's a major expression

@TPGamesNL TPGamesNL added 2.5 bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. labels Feb 19, 2021
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

epic pr 😎
I recommend adding a test just in case (it wouldn't hurt to have). I think it would be okay to split it up like discussed in the comments. Another review from another developer would be appreciated.

Copy link
Member

@FranKusmiruk FranKusmiruk left a comment

Choose a reason for hiding this comment

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

Looks pretty good. It could make use of encapsulation in some places, but that's more of a nitpick than anything.

@FranKusmiruk
Copy link
Member

  • whether the chain parsing should be done on init, or in the first evaluation (since doing it in init is useless for any inner ExprArithmetics, which won't be if it's called in get, with the tradeoff of having to parse it on runtime)

I would do it on initialization if possible, i.e.: literals or if we can simplify the expression. In other cases, parsing can be done at runtime.

@FranKusmiruk FranKusmiruk added 2.6 and removed 2.5 labels Mar 3, 2021
@TPGamesNL TPGamesNL changed the base branch from master to dev-2.6 March 3, 2021 16:01
@FranKusmiruk FranKusmiruk changed the base branch from dev-2.6 to master March 4, 2021 08:22
@FranKusmiruk FranKusmiruk merged commit 6ff7df1 into SkriptLang:master May 3, 2021
@TPGamesNL TPGamesNL deleted the fix/math-order branch May 3, 2021 10:00
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