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

Closures: Added tests from the current closures PR #1393

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

torch2424
Copy link
Contributor

@torch2424 torch2424 commented Jul 16, 2020

relates to #1308
relates to https://github.com/AssemblyScript/working-group/issues/41

As discussed in the working group meeting, this moves the Tests that are being added / modified in the current AssemblyScript Closures PR #1308 , and skips the relevant errors in these closures tests. This is being done, as these tests represent our current goals of what closures should support, while also, allowing us to start bringing closures into main/master in smaller more manageable PRs 馃槃

NOTE: Closures will need additional tests later down the line. But again, just trying to start merging in what we currently have in parts that won't affect the project as a whole or break things 馃槃

cc @DuncanUszkay1 馃槃

Screenshots

Screenshot from 2020-07-15 17-41-40
Screenshot from 2020-07-15 17-39-36

@torch2424 torch2424 requested a review from dcodeIO July 16, 2020 00:45
@torch2424 torch2424 self-assigned this Jul 16, 2020
Comment on lines 1 to 12
function add(x: i32, y: i32): i32 {
return x + y;
}
function apply_to_pair(x: i32, y: i32, fn: (x:i32, y:i32) => i32): i32 {
return fn(x,y);
}

let addResult = add(1, 1);
let applyResult = apply_to_pair(1, 2, add);

assert(addResult == 2);
assert(applyResult == 3);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the closure in this test 馃 what is the purpose of this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Good catch, yeah we added this test because of our current implementation in #1308 had a problem with this test. But, this was before we fixed the CI, so this test probably isn't needed. I'll remove it 馃槃

@torch2424
Copy link
Contributor Author

@dcodeIO @DuncanUszkay1 This should be good to go now! Tests are passing and everything 馃槃 馃憤

@dcodeIO
Copy link
Member

dcodeIO commented Jul 21, 2020

Thanks Aaron! Suggesting to leave this PR open until we have an implementation to test positive against. Still good to have these tests pinned here so we know what we are looking for initially :)

@torch2424
Copy link
Contributor Author

@dcodeIO You are welcome! 馃槃

Suggesting to leave this PR open until we have an implementation to test positive against. Still good to have these tests pinned here so we know what we are looking for initially :)

So normally I would agree, but since the PR is going to be really really large. I think it'd probably good to merge these, and then unskip / update them in our final closures PR? If you are worried that people will think that closures will work, I can add a note comment on the top of the file? 馃槃

I also think this method would be a better, that way we can implement closures themselves in smaller pieces and get tests passing one or a couple at a time 馃槃

Unless you would prefer to just merge it in an all-in-one PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants