Skip to content

Add "Time Until" to ExprTimeSince and Add CondPastFuture#6203

Merged
sovdeeth merged 17 commits into
SkriptLang:dev/featurefrom
sovdeeth:time-until
Oct 13, 2024
Merged

Add "Time Until" to ExprTimeSince and Add CondPastFuture#6203
sovdeeth merged 17 commits into
SkriptLang:dev/featurefrom
sovdeeth:time-until

Conversation

@sovdeeth
Copy link
Copy Markdown
Member

Description

Continuation of #5767 as previous author did not want to continue working on it.

Also adds CondPastFuture for testing whether a date is in the past or the future. This is simply a more intuitive syntax for if {_x} < now that also supports multiple dates:

%dates% (is|are)[n't| not] in the (past|future)
%dates% ha(s|ve)[n't| not] passed

This has the issue of being unreliable when using ExprNow (who would do that) so there's an explicit check for ExprNow instead of relying on chance for the two created dates to be the same. I added a check for ExpressionLists with ExprNow in them, but I'm not sure that's worth checking.


breaking change of returning 0 seconds for out-of-range dates instead of null:

Previously I disagreed with this change, as seen here:

My view remains that changing the established behavior doesn't really make sense here; there's no real benefit to 0 timespan vs null imo and the change in behavior isn't worth the slight improvement in intuition.

However, I've thought more about it and I think it has benefits outside of intuition. Returning 0 seconds instead of null allows comparisons to be more reliable, since the returned value is still a timespan and will evaluate accordingly instead of being null and being related to negation.

if time since {_date} not is less than 3 seconds:
# Previously:
# if {_date} is in the future, true. This is kind of strange in my opinion. No time has passed since a future date, so the time since should be less than 3 seconds.
# if {_date} is more than 3 seconds ago, true.
# if {_date} is between 0 and 3 seconds ago, false.
# Now:
# if {_date} is in the future, false.
# if {_date} is more than 3 seconds ago, true.
# if {_date} is between 0 and 3 seconds ago, false.

However, it does cause some significant change. Users may rely on the future date to be null, for example using if time since x to test if a date is in the past instead of using if x is less than now.

if time since {_date} is less than 3 seconds:
# Previously:
# if {_date} is in the future, false.
# if {_date} is more than 3 seconds ago, false.
# if {_date} is between 0 and 3 seconds ago, true.
# Now:
# if {_date} is in the future, true.
# if {_date} is more than 3 seconds ago, false.
# if {_date} is between 0 and 3 seconds ago, true.

More opinions are welcome!


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

@sovdeeth sovdeeth added enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature Pull request adding a new feature. up for debate When the decision is yet to be debated on the issue in question breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) 2.8 labels Nov 24, 2023
Comment thread src/main/java/ch/njol/skript/conditions/CondPastFuture.java
Comment thread src/main/java/ch/njol/skript/expressions/ExprTimeSince.java Outdated
Comment thread src/test/skript/tests/syntaxes/expressions/ExprTimeSince.sk Outdated
@sovdeeth sovdeeth added 2.9 and removed 2.8 labels Dec 31, 2023
Copy link
Copy Markdown
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

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

Good, maybe should have been 2 separate PRs though

@APickledWalrus APickledWalrus removed the 2.9 label Jul 2, 2024
Comment thread src/main/java/ch/njol/skript/conditions/CondPastFuture.java Outdated
Comment thread src/main/java/ch/njol/skript/conditions/CondPastFuture.java Outdated
Comment thread src/main/java/ch/njol/skript/conditions/CondPastFuture.java Outdated
Comment thread src/main/java/ch/njol/skript/expressions/ExprTimeSince.java Outdated
Comment thread src/main/java/ch/njol/skript/expressions/ExprTimeSince.java Outdated
Comment thread src/main/java/ch/njol/skript/expressions/ExprTimeSince.java Outdated
@sovdeeth sovdeeth requested a review from Efnilite September 10, 2024 22:50
@sovdeeth sovdeeth added feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. and removed up for debate When the decision is yet to be debated on the issue in question labels Sep 15, 2024
}

// using the same 'now' date for all checks is flawed, because the input dates are evaluated during the
// check, so it could cause 'now is in the future' to be true, when it should be false.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't the now case already checked above though?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes this was written prior to the ExprNow checks, but technically still applicable with other scenarios

@sovdeeth sovdeeth merged commit 33ebb8f into SkriptLang:dev/feature Oct 13, 2024
erenkarakal pushed a commit to erenkarakal/Skript that referenced this pull request Nov 26, 2025
…6203)

* Time Until

* ghost of christmas past

Adds condition for checking whether dates are in the past or the future

* Update CondPastFuture.java

* Update CondPastFuture.java

* Make tests more reliable

* Requested changes

* requested changes, fix merge issue

---------

Co-authored-by: Moderocky <admin@moderocky.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature Pull request adding a new feature. 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.

5 participants