-
-
Notifications
You must be signed in to change notification settings - Fork 498
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
Standardize Timeout setter for wait strategies #322
Conversation
Codecov Report
@@ Coverage Diff @@
## master #322 +/- ##
==========================================
- Coverage 60.91% 57.59% -3.32%
==========================================
Files 15 13 -2
Lines 985 915 -70
==========================================
- Hits 600 527 -73
- Misses 293 312 +19
+ Partials 92 76 -16
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Please check my comment about deprecation logs, as I would like to know your thoughts
Thanks for addressing this issue!
@mniak I have one NIT (maybe personal OCD): would it be possible to add meaningful commit messages? We are currently merging the PR branch into the master branch, although with current messages I'd vote for squashing these ones into one. @gianarb do you think we need more elaborated info in a contribution guide? |
Sorry, @mdelapenya. I'm very used to always merging PRs with the "squash and merge" button and I forgot this is not a project of mine. I will squash it here by myself. I think that enforcing squash-and-merge is more reliable than checking the phrasing of the commit messages of contributors, since the final approver can change the phrasing by himself in one atomic commit that represents the entire set of changes in that PR. Seems to me that squash-and-merge is easier for everyone: contributors and approvers |
Indeed. I totally agree, that's why I summoned @gianarb for the need for a contribution guide that simplifies the onboarding experience for contributors. And maybe moving to the squash model makes sense here, but would like to check with this great community first. |
ebfb9ab
to
308c258
Compare
Had to move the file When I tried to add a unit test for checking the timeout setters in the HTTP strategy, I found that this file used a differente package I then moved the file |
@mniak could you 🙏 rebase and resolve conflicts? We solved (actually skipped) the |
Hey @mniak could your merge with upstream and resolve conflicts 🙏 ? Sorry that review took so long that conflicts appeared 😞 Let's try to make this happen |
Hi @mniak 👋 I'm revisiting this stale PR as we'd like to add it to the next release, and wonder if you are interested in resolving the conflicts. wdyt? |
Closes #314
WithTimeout
.