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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃殌 Making function param rules stricter for the better #6361

Merged

Conversation

AyhamAl-Ali
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali commented Jan 22, 2024

Description

Today is another day making Skript making more sense 馃檭

Old Behaviour

After playing around with #6358 we found out that function parameters actually have almost zero limitation and @sovdeeth was generous enough to provide the weirdest valid param examples, enjoy it:

function testA(loc): location, previoustype(): text):
    broadcast "a"

function testB(loc(: location, previoustype): text):
    broadcast "b"

function testE(loc: location, previoustyp): returns text: object = (")")):
    broadcast "%{_loc}% %{_previoustyp): returns text}%"

function this((function should(not: exist) returns text:) ? object : text) :: text:
    return {_(function should(not: exist) returns text:) ? object }

function never(gonna: give = (you - up)): and function never(gonna: let, you: down) :: text):
    broadcast {@magic}

function testA3(previoustyp): returns text: object = (")")):
	broadcast 2

Actually, these all make no sense so we decided to rectify them :)

New Behaviour

Welcome to the new function param rules

It is correct that variables do accept many forms of names but it's very odd to have those forms in function param and due to the parser limitation we are better off stricting this to make it easier for the parser.

Functions parameters will now only accept Alphanumeric, whitespace, period, underscore and a dash (regex: [\w .-])

Regex101 playground

How they error now:
image


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

@AyhamAl-Ali AyhamAl-Ali added enhancement Feature request, an issue about something that could be improved, or a PR improving something. breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) labels Jan 22, 2024
@TheLimeGlass
Copy link
Collaborator

The test that needs to be fixed, still errors

 [23:27:11 INFO]: [Skript] Line 4: (../../../../../../src/test/skript/tests/syntaxes/expressions/ExprStringCase.sk)
[23:27:11 INFO]:     List is missing 'and' or 'or', defaulting to 'and': "Oops!", "OOPS!"
[23:27:11 INFO]:     Line: assert caseEquals(capitalised "Oops!", "OOPS!") is true with "capitalised failed"

@AyhamAl-Ali
Copy link
Member Author

The test that needs to be fixed, still errors

 [23:27:11 INFO]: [Skript] Line 4: (../../../../../../src/test/skript/tests/syntaxes/expressions/ExprStringCase.sk)
[23:27:11 INFO]:     List is missing 'and' or 'or', defaulting to 'and': "Oops!", "OOPS!"
[23:27:11 INFO]:     Line: assert caseEquals(capitalised "Oops!", "OOPS!") is true with "capitalised failed"

This wasn't within the scope of this PR. Though what's the behaviour expected in that case?

@sovdeeth sovdeeth added the 2.9 Targeting a 2.9.X version release label Feb 2, 2024
@AyhamAl-Ali AyhamAl-Ali added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Mar 15, 2024
@AyhamAl-Ali AyhamAl-Ali marked this pull request as draft March 27, 2024 18:26
@AyhamAl-Ali AyhamAl-Ali marked this pull request as ready for review April 5, 2024 04:46
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.

nice work

@Moderocky Moderocky merged commit cecf0b4 into SkriptLang:dev/feature May 8, 2024
5 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 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-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.

None yet

5 participants