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

Addd waitForPending update method #73

Merged
merged 1 commit into from
Oct 26, 2020
Merged

Addd waitForPending update method #73

merged 1 commit into from
Oct 26, 2020

Conversation

ppamorim
Copy link
Contributor

This PR removes the usage of Thread.sleep(timeInterval:) included between async calls. I tried to implement a solution based on DispatchSemaphore but this turns out to be problematic when used inside closures and it was creating a deadlock.

@ppamorim ppamorim added enhancement New feature or request Hacktoberfest labels Oct 10, 2020
@ppamorim ppamorim requested a review from curquiza October 10, 2020 21:00
@ppamorim ppamorim self-assigned this Oct 10, 2020
@ppamorim
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Oct 10, 2020
@bors
Copy link
Contributor

bors bot commented Oct 10, 2020

try

Build succeeded:

@ppamorim ppamorim requested a review from bidoubiwa October 12, 2020 12:38
Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

I changed two things in this review

  1. As the update routes give information about a process and not a result, I changed ResultType for ProcessType.
  2. In all other SDK's it is waitForPendingUpdate and not waitForPendingUpdate so I changed it! :)

@ppamorim ppamorim requested a review from bidoubiwa October 14, 2020 00:52
bidoubiwa
bidoubiwa previously approved these changes Oct 14, 2020
@curquiza
Copy link
Member

Should be merged after #71

@curquiza
Copy link
Member

curquiza commented Oct 19, 2020

@ppamorim, there are conflicts, sorry but we cannot use Bors here for the moment 😕 You need to rebase your branch.

@ppamorim
Copy link
Contributor Author

@curquiza I will do this after we merge #72.

@ppamorim ppamorim force-pushed the feature/pooling branch 2 times, most recently from f8bc033 to 2ecf83f Compare October 23, 2020 14:28
@ppamorim
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Oct 23, 2020
@bors
Copy link
Contributor

bors bot commented Oct 23, 2020

try

Build succeeded:

@ppamorim ppamorim requested a review from bidoubiwa October 23, 2020 15:18
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

Great addition! Thanks!

@curquiza curquiza changed the title Remove the use of Thread.sleep Addd waitForPending update method Oct 26, 2020
@curquiza
Copy link
Member

bors merge

@curquiza
Copy link
Member

curquiza commented Oct 26, 2020

@ppamorim is this PR breakable because you change public let number: Int into public let number: Int? in Update model? Not a big deal, just to know.

@bors
Copy link
Contributor

bors bot commented Oct 26, 2020

Build succeeded:

@bors bors bot merged commit 0af420d into master Oct 26, 2020
@bors bors bot deleted the feature/pooling branch October 26, 2020 15:57
@ppamorim
Copy link
Contributor Author

@curquiza This is a required change because the update result may not return the number field. Before this the test was crashing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants