Conversation
Absolutionism
requested review from
APickledWalrus and
Efnilite
and removed request for
a team
sovdeeth
left a comment
sovdeeth
left a comment
There was a problem hiding this comment.
i will look more closely later
sovdeeth
left a comment
sovdeeth
left a comment
There was a problem hiding this comment.
everything else looks good, though there might be an opportunity for optimization if the left expression has a return value that exactly matches an operation, then you wouldn't have to do lookups during the function. Optional, though.
APickledWalrus
left a comment
APickledWalrus
left a comment
There was a problem hiding this comment.
Here are some initial things I noticed
|
|
||
| test "operations effect invalid operation": # TODO: test for runtime errors | ||
| set {_date} to now | ||
| multiply {_date} by 2 |
There was a problem hiding this comment.
should be asserting the value of {_date} afterwards
| error("Cannot operate with a null object."); | ||
| return; | ||
| } | ||
| if (left.isSingle() && left.getSingle(event) == null) { | ||
| error("Cannot operate on a null object."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Shouldn't be doing errors here imo. Precedent is for effects acting on nothing to simply do nothing.
There was a problem hiding this comment.
I might like to see us experiment with having this piggyback off of ExprArithmetic. That is, you create a new ExprArithmetic and initialize it with the associated matched pattern and such. You can then just evaluate the expression in execute and change the value. If there is some major reason this wouldn't (or doesn't) work, let me know. OR, it might be better to just pull out some of the shared logic for unparsed literal/return type handling
Also, it might not hurt to have an arithmetic module :) would like to hear what others think about that though (would also provide organization for my proposal above)
| divide 10 by {_num} | ||
| """) | ||
| @Since("INSERT VERSION") | ||
| public class EffOperations extends Effect implements SyntaxRuntimeErrorProducer { |
skriptlang-automation
Bot
added
needs reviews
APickledWalrus
left a comment
APickledWalrus
left a comment
There was a problem hiding this comment.
Looking pretty good. I'm still wondering if we need to mimic the UnparsedLiteral logic from ExprArithmetic, but I'll get back to you on that.
| Object rightObject = right.getSingle(event); | ||
| if (rightObject == null) { | ||
| error("Cannot operate with a null object."); | ||
| return; | ||
| } | ||
| if (left.isSingle() && left.getSingle(event) == null) { | ||
| error("Cannot operate on a null object."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I'm not sure if we'd want runtime errors or not for null values (would like to hear what others think). Some operations/types have default values... do we need to be checking those here?
There was a problem hiding this comment.
Since no one has replied, should I take it that it's fine to have?
There was a problem hiding this comment.
it should act identically to how set {_x} to {_x} * {_y} works
whatever that behavior is should be mirrored.
|
Closing due to inactivity |
6 participants
Description
This PR aims to add effects for multiplication, division and exponentiation operations, allowing users to perform an arithmetic operation on an variable, single or list. Rather than doing, for example
set {_int} to {_int} * 2Does not work for literals.
Target Minecraft Versions: any
Requirements: none
Related Issues: #7763