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

Support array indexing by range II #634

Merged
merged 3 commits into from
Dec 18, 2024

Conversation

hdwalters
Copy link
Contributor

Remove failing negative index validity tests.

Copy link
Member

@mks-h mks-h left a comment

Choose a reason for hiding this comment

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

I don't think we should remove these tests, as they don't break our CI. Plus, they will be useful when we decide to fix the issue.

Maybe instead of removing them altogether, you can extract them into separate files.

src/modules/variable/get.rs Outdated Show resolved Hide resolved
@hdwalters
Copy link
Contributor Author

hdwalters commented Dec 15, 2024

Maybe instead of removing them altogether, you can extract them into separate files.

I can certainly do that, but I think I also need to skip the tests in those files if the Bash version is not high enough.

See this Discord discussion about the various options.

@hdwalters
Copy link
Contributor Author

hdwalters commented Dec 15, 2024

I've pushed a new commit, to split the failing tests into ones for positive and negative integers, and made the negative tests succeed on earlier Bash versions. This requires a change to the test framework, where we pass automatically if the output is "Succeeded", even in the presence of an expected output comment.

@Mte90
Copy link
Member

Mte90 commented Dec 16, 2024

@hdwalters there are conflicts

@hdwalters
Copy link
Contributor Author

According to https://mywiki.wooledge.org/BashFAQ/061, this functionality was introduced in Bash 4.2. I will adjust the workaround in the test scripts.

@hdwalters hdwalters force-pushed the array-indexing-by-negative branch 2 times, most recently from 0d97818 to b71bda7 Compare December 16, 2024 20:31
@hdwalters
Copy link
Contributor Author

I've fixed the merge conflicts, and enabled the negative index tests for Bash 4.2 and above; I would be grateful for another review please.

@hdwalters hdwalters requested a review from mks-h December 16, 2024 20:53
@hdwalters hdwalters force-pushed the array-indexing-by-negative branch from e11415f to 531213a Compare December 16, 2024 21:12
@Mte90
Copy link
Member

Mte90 commented Dec 17, 2024

there is a test failing

@hdwalters
Copy link
Contributor Author

there is a test failing

That's nothing to do with my changes. Looks like a Heisenbug to me.

@hdwalters
Copy link
Contributor Author

hdwalters commented Dec 17, 2024

there is a test failing

That's nothing to do with my changes. Looks like a Heisenbug to me.

I reran the tests, and they worked the second time. So I think it's a Heisenbug, given that the tests were failing in a completely different area to my changes (in the input_hidden function).

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

Thanks @hdwalters, this looks good!

@Ph0enixKM Ph0enixKM merged commit 3d5c47f into amber-lang:master Dec 18, 2024
1 check passed
@hdwalters hdwalters deleted the array-indexing-by-negative branch December 18, 2024 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants