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

Make function parameters respect the case-insensitive-variables setting. #6388

Merged

Conversation

sovdeeth
Copy link
Member

@sovdeeth sovdeeth commented Feb 2, 2024

Description

Changes Parameter#parse() to only compare lowercase parameter names if caseInsensitive is true.
Changes Parameter to store the original string, rather than the lowercased string.

Manual testing indicates this fixes the issue.

Technically breaking changes in a very niche scenario, but only because it's reverting to intended behavior, so as previously established, not marking as breaking changes.


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

@sovdeeth sovdeeth added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. 2.8 Targeting a 2.8.X version release labels Feb 2, 2024
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

As you mentioned it's not really a breaking change but in a scenario where a user had a similar variable usage to the code below

function test(TEST: text):
    broadcast {_test}

their code will sadly not work if they had case-insensitive-variables as false. Though it might be rare to happen but I highly recommend making a statement about this in the next release just in case.

@sovdeeth
Copy link
Member Author

sovdeeth commented Feb 2, 2024

As you mentioned it's not really a breaking change but in a scenario where a user had a similar variable usage to the code below

function test(TEST: text):
    broadcast {_test}

their code will sadly not work if they had case-insensitive-variables as false. Though it might be rare to happen but I highly recommend making a statement about this in the next release just in case.

yeah good idea, should definitely mention it.

Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Is a test for this possible? Maybe could do something like

test "parameter test":
  assert parameterTest("test") is "test" with "did not return correct value"
function parameterTest(text: text) :: text:
    return {_text}

@sovdeeth
Copy link
Member Author

sovdeeth commented Feb 2, 2024

Is a test for this possible? Maybe could do something like

test "parameter test":
  assert parameterTest("test") is "test" with "did not return correct value"
function parameterTest(text: text) :: text:
    return {_text}

I suppose, but we can't really test the actual point of the change, which requires the config option to be set to false.
Such an example wouldn't really help test anything but normal function behavior.

@sovdeeth sovdeeth added the patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. label Feb 2, 2024
@APickledWalrus
Copy link
Member

Is a test for this possible? Maybe could do something like

test "parameter test":
  assert parameterTest("test") is "test" with "did not return correct value"
function parameterTest(text: text) :: text:
    return {_text}

I suppose, but we can't really test the actual point of the change, which requires the config option to be set to false. Such an example wouldn't really help test anything but normal function behavior.

lol i just realized

@Pikachu920
Copy link
Member

Is a test for this possible? Maybe could do something like

test "parameter test":
  assert parameterTest("test") is "test" with "did not return correct value"
function parameterTest(text: text) :: text:
    return {_text}

I suppose, but we can't really test the actual point of the change, which requires the config option to be set to false. Such an example wouldn't really help test anything but normal function behavior.

would it be possible to do so by changing the config option through a junit test?

@sovdeeth sovdeeth merged commit 7f61018 into SkriptLang:dev/patch Feb 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 Targeting a 2.8.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.

None yet

5 participants