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

Return isNegative() and not false, if CondIsWithin is invalid #7038

Merged
merged 3 commits into from
Sep 22, 2024

Conversation

TenFont
Copy link
Contributor

@TenFont TenFont commented Sep 5, 2024

Description

This PR fixes the fact that CondIsWithin always returns false if:

  • either of the two locations in is within %location% and %location is null, or if both are in separate worlds
  • if the "container" is null in is within %entity/chunk/world/block%

Now, it takes into account whether the condition is negated, and returns true or false appropriately.

What was the issue?

Say I have a guard clause like so:

if player is not within {_loc1} and {_loc2}:
  send "&cThe region specified is invalid."
  stop
# do something

If the condition fails, one would assume that the player is within the two locations. However, this precondition is violated if either of the two locations are not set. As the condition is doomed to always fail if the locations are null (regardless of negation), the following code will still run. This PR fixes this.


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

@sovdeeth
Copy link
Member

sovdeeth commented Sep 5, 2024

I'm not sure if I like this approach. A player not being in a region implies the region exists to begin with. I think the current behavior matches better with established convention of NaN always returning false when compared to other numbers. That said, I'm not sure what behavior is most common within Skript's conditions.

@TenFont
Copy link
Contributor Author

TenFont commented Sep 5, 2024

I'm not sure if I like this approach. A player not being in a region implies the region exists to begin with. I think the current behavior matches better with established convention of NaN always returning false when compared to other numbers. That said, I'm not sure what behavior is most common within Skript's conditions.

CondContains returns isNegated() if the container is not set, not false. Current behaviour of CondIsWithin is inconsistent.

@sovdeeth
Copy link
Member

sovdeeth commented Sep 5, 2024

Yeah looking at SimpleExpression#check() it does seem precedent to return negated if null valued.
I think that's probably a poor decision in most cases but it's better to be consistent, so ignore my concerns for this PR.

@sovdeeth sovdeeth added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. 2.9 Targeting a 2.9.X version release labels Sep 7, 2024
@sovdeeth sovdeeth merged commit 9801329 into SkriptLang:dev/patch Sep 22, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.9 Targeting a 2.9.X version release bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants