fix bug when writing to foreign proc #771
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Moving back into draft to address #770, since it is a highly related issue. |
Yaiba
requested changes
May 28, 2024
| var initialData = map[string][]string{ | ||
| "satoshi": {"hello world", "goodbye world", "buy $btc to grow laser eyes"}, | ||
| "zeus": {"i am zeus", "i am the god of thunder", "i am the god of lightning"}, | ||
| "wendys_drive_through_lady": {"hi how can I help you", "no I don't know what the federal reserve is", "sir this is a wendys"}, |
parse/analyze.go
Outdated
| returns2, ok := p0.Call.Accept(p).([]*types.DataType) | ||
| if !ok { | ||
| p.errs.AddErr(p0.Call, ErrType, "expected function/procedure to return one or more variables") | ||
| } else if returns2, ok := returnVal.([]*types.DataType); ok { |
There was a problem hiding this comment.
probably should consider refactor this using switch?
parse/analyze.go
Outdated
|
|
||
| _, ok = e.Accept(s).(*returnsTable) |
There was a problem hiding this comment.
looks like you can refactor this function switch
| foreign: `foreign procedure do_something()`, | ||
| otherProc: `procedure call_foreign() public { | ||
| do_something['%s', 'delete_users'](); | ||
| }`, |
There was a problem hiding this comment.
what's the expected result? expect no err and no outputs?
There was a problem hiding this comment.
nothing. No error and no outputs, as the procedure should be successful and it does not return anything.
| wantErr: "requires 1 arg(s)", | ||
| }, | ||
| { | ||
| name: "foreign procedure returns 1 arg, implementation returns none", |
There was a problem hiding this comment.
As you pointed out, the test name since here are less confusing. It would be better we change above test case name
|
Ok I addressed the changes @Yaiba |
updated comment fix bugs on foreign procedures fix build tag fixed act test addressed gavins feedback fix failing unit test
Yaiba
approved these changes
May 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes several bugs, as well as adds better error messages and adds many tests for foreign procedures.
It fixes the bug found by @KwilLuke where writing data to a foreign procedure failed, as well as #770. It adds tests for both of them. It also adds many negative test cases to check for error messages thrown during runtime.