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

Include Example and Test for Promises Created in For Loops #309

Merged
merged 5 commits into from
Nov 28, 2022

Conversation

idea404
Copy link
Contributor

@idea404 idea404 commented Nov 23, 2022

This PR includes a failing example for promises created in for loops

@idea404 idea404 added the bug Something isn't working label Nov 23, 2022
@idea404 idea404 requested a review from ailisp as a code owner November 23, 2022 14:21
@idea404 idea404 self-assigned this Nov 23, 2022
@idea404 idea404 requested a review from volovyks as a code owner November 23, 2022 14:21
@idea404
Copy link
Contributor Author

idea404 commented Nov 23, 2022

I believe the CI tests the current code against the existing package.json? Which would be why tests are passing here?

In my local env this is the test output:

image

@petarvujovic98 also, any clue why the terminal output is printed line by line now like this when building?

@gagdiez
Copy link
Collaborator

gagdiez commented Nov 23, 2022

@idea404 promise.then and promise.and do not append to the variable promise, but return a new NearPromise, this means that:

const promise = NearPromise.new(account)
promise.functionCall(something) /// here you are missing the returned object
return promise // you are returning the result of doing `NearPromise.new()` which is 0 apparently.

What you meant to do is:

let promise = NearPromise.new(contracts[0]).functionCall()

for i in 1...3 {
   promise = promise.and( NearPromise.new(contracts[i]).functionCall() )
}

promise = promise.then( callback )

return promise.asReturn()

Also, in the callback you are not currently returning the counter value.

Applying all those changes make the tests pass.

@idea404
Copy link
Contributor Author

idea404 commented Nov 24, 2022

@gagdiez Perfect! Tests now do pass. I'll leave merging this PR here as an example of how to loop promises to one another to the discretion of the maintainers.

@gagdiez
Copy link
Collaborator

gagdiez commented Nov 24, 2022

one last thing I would suggest is removing most logs and the variable callCount, in the callback you can simple pass or use the array length to know the number of calls.

@petarvujovic98
Copy link
Contributor

petarvujovic98 commented Nov 24, 2022

@idea404 I see that the CLI has been updated with new commands, and in doing so there was a degression where instead of creating a new signale instance for each command they are using a global one. This is the commit that changed this - line 46 to be exact. The solution for this would be to simply create a new signale instance for each command function with the interactive option configured the same way as it was in the build command, and of course changing the scope to be the command name and to actually use those instances in logging instead of the global variable.

@petarvujovic98 petarvujovic98 mentioned this pull request Nov 24, 2022
@petarvujovic98
Copy link
Contributor

I've addressed the logging issue in #310 .

Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

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

Very good example! Thanks @idea404 and @petarvujovic98

@ailisp
Copy link
Member

ailisp commented Nov 28, 2022

Oh @idea404 you need to add the test to ci, in .github/workflows/examples.yml

@volovyks
Copy link
Collaborator

Thank you @idea404 !

@volovyks volovyks merged commit b0b0f50 into develop Nov 28, 2022
@idea404 idea404 deleted the xcc-loop branch November 29, 2022 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants