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

actor/task .call are missing waitSecs option #282

Closed
metalwarrior665 opened this issue Oct 27, 2022 · 3 comments · Fixed by #283
Closed

actor/task .call are missing waitSecs option #282

metalwarrior665 opened this issue Oct 27, 2022 · 3 comments · Fixed by #283
Assignees
Labels
bug Something isn't working.

Comments

@metalwarrior665
Copy link
Member

The type ActorStartOptions has waitForFinish but actor('actor').call (and task) where it is used should have waitSecs instead. I would do a PR but not sure how you want to structure the types

@metalwarrior665 metalwarrior665 added the bug Something isn't working. label Oct 27, 2022
@vladfrangu
Copy link
Member

Out of curiosity, why is that? The api expects waitForFinish, not waitSecs. Is this just a rename in the client for clarity sake or is it something else?

@metalwarrior665
Copy link
Member Author

metalwarrior665 commented Nov 1, 2022

@vladfrangu I think this was designed by @mnmkng but as I understand it: The .start() is basically the normal client run so it passes all the params to the API (waitForFinish is API param but it waits max 300 seconds to not block the connection). But .call() is a wrapper of .start() that waits for the run to finish fully. This is feature of the client, not supported by the API so the waitSecs is option only to the client, not API.

I agree that it is a bit confusing since vast majority of the client just reflects the API but we generally agreed that the client should have utility logic on top of the API.

@vladfrangu
Copy link
Member

Oh crap, I think I see what you're saying. .call re-uses the type for .start but in the case of waitSecs vs waitForFinish, .call expects the former, and .start the latter. I'll fix this, thx

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 a pull request may close this issue.

2 participants