Skip to content

Allow Literals in 'Size of' #7961

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

Conversation

erenkarakal
Copy link
Member

@erenkarakal erenkarakal commented Jun 19, 2025

Problem

There is an extremely old check that stops Literals from being used in size of expression which recently became an issue in my Timezone Support PR. After discussion we decided to euthanize it

There is also another bug with ExprAmount that makes {a} count towards recursive size of {a::*} which was fixed in this PR

Solution

Removes the check that stops Literals from being used

Caution

Breaking change:
when using size of {a::*}, "b":
old behavior (size of {a::*}), "b"
new behavior size of ({a::*}, "b")

Testing Completed

ExprAmount.sk

Supporting Information


Completes: none
Related: none

@erenkarakal erenkarakal requested a review from a team as a code owner June 19, 2025 20:29
@erenkarakal erenkarakal requested review from cheeezburga and UnderscoreTud and removed request for a team June 19, 2025 20:29
@skriptlang-automation skriptlang-automation bot added the needs reviews A PR that needs additional reviews label Jun 19, 2025
@sovdeeth
Copy link
Member

This may cause some breaking changes with things like set {_x::*} to amount of {_y::*} and 2
Some tests to confirm would be good

@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Jun 19, 2025
Copy link
Contributor

@Burbulinis Burbulinis left a comment

Choose a reason for hiding this comment

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

looks good to me, you can replace the switch with an enhanced switch though

@skriptlang-automation skriptlang-automation bot removed the needs reviews A PR that needs additional reviews label Jun 22, 2025
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.

still needs some tests to check if behavior changed! eg (size of {a::*} and 1)

erenkarakal and others added 3 commits June 26, 2025 18:55
Co-authored-by: Patrick Miller <apickledwalrus@icloud.com>
Co-authored-by: Patrick Miller <apickledwalrus@icloud.com>
Co-authored-by: Patrick Miller <apickledwalrus@icloud.com>
@erenkarakal erenkarakal added breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) labels Jun 26, 2025
@skriptlang-automation skriptlang-automation bot added the patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. label Jul 1, 2025
@APickledWalrus APickledWalrus moved this from In Review to Awaiting Merge in 2.12 Release Jul 1, 2025
@APickledWalrus APickledWalrus merged commit 83198b1 into SkriptLang:dev/patch Jul 1, 2025
5 checks passed
@github-project-automation github-project-automation bot moved this from Awaiting Merge to Done in 2.12 Release Jul 1, 2025
@skriptlang-automation skriptlang-automation bot added completed The issue has been fully resolved and the change will be in the next Skript update. and removed patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. labels Jul 1, 2025
Burbulinis pushed a commit to Burbulinis/Skript that referenced this pull request Jul 9, 2025
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.) completed The issue has been fully resolved and the change will be in the next Skript update. enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants