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

Fix continueWaitTimout is ignored #224

Merged
merged 2 commits into from
Oct 4, 2019

Conversation

vieira
Copy link
Contributor

@vieira vieira commented Oct 3, 2019

Check List

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Description of Changes Made
I am not familiar with the internals of cube, however I am experiencing a problem where continueWaitTimeout is not being honored and upon inspecting the code it seems that the option is only used as a fallback to an argument that is never undefined.

Furthermore it seems thar a pair of parentheses is missing (?) otherwise it will wait for 5ms instead of 5s.

I might be missing something as I am really not familiar with the code, please review.

Thank you.

@paveltiunov
Copy link
Member

@vieira Hey João! Thanks for contributing this! Nice catch! I believe continueWaitTimeout can be completely removed as argument. Seems It's just some code refactoring glitch.

@vieira
Copy link
Contributor Author

vieira commented Oct 4, 2019

Thanks @paveltiunov, makes sense. Just pushed another commit to remove the argument.

@paveltiunov
Copy link
Member

@vieira Nice! Could you please rebase your branch to fix tests so we're good to merge? Thanks!

@vieira vieira force-pushed the honor-continueWaitTimeout-local branch from 530cbbe to 274ae18 Compare October 4, 2019 19:46
@vieira
Copy link
Contributor Author

vieira commented Oct 4, 2019

@paveltiunov Sure, done!

@paveltiunov
Copy link
Member

@vieira Cool! Thanks!

@paveltiunov paveltiunov merged commit 4f72a52 into cube-js:master Oct 4, 2019
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.

2 participants