Allow CapacityLimiter(math.inf) #728
Conversation
|
Awesome! The change looks good. You have a test failure with Travis, and it looks like it's just due to bad formatting: https://travis-ci.org/python-trio/trio/jobs/438987364 Also, could you add a release note for this change? https://trio.readthedocs.io/en/latest/contributing.html#pull-request-release-notes |
|
@merrellb ping! still interested in working on this? |
|
I am. Somehow missed your earlier reply. Will take a closer look tomorrow.
…On Thu, Oct 18, 2018 at 9:12 PM Nathaniel J. Smith ***@***.***> wrote:
@merrellb <https://github.com/merrellb> ping! still interested in working
on this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#728 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAS5exVbBzOD7ViQK8ZZMwVBzyvoVjOVks5umScBgaJpZM4XRjHv>
.
|
Codecov Report
@@ Coverage Diff @@
## master #728 +/- ##
==========================================
+ Coverage 99.34% 99.34% +<.01%
==========================================
Files 96 96
Lines 11577 11588 +11
Branches 827 827
==========================================
+ Hits 11501 11512 +11
Misses 56 56
Partials 20 20
Continue to review full report at Codecov.
|
|
I've fixed the yapf formatting but the newsfragment seems to be causing an error in Jenkins. When I click on |
|
Closing and reopening to see if that fixes the issue. |
|
That did not help, sorry. |
|
Thanks anyway :-) Are there other steps worth trying or should I just close and resubmit the pull request? |
|
Looks like the Jenkins server is having some trouble... I pinged the admins. |
|
The issue with viewing the Jenkins logs is: #749 The build failure was some random nonsense, like a network timeout or something. Re-running the build fixed it. |
|
(But unfortunately, due to the issues described in #749, I'm the only one who can re-run a build right now so... yeah. Jenkins is frustrating.) |
| @@ -115,6 +115,21 @@ | |||
| c.release_on_behalf_of("value 1") | |||
|
|
|||
|
|
|||
| async def test_CapacityLimiter_inf(): | |||
There was a problem hiding this comment.
The mixed CamelCase here probably breaks PEP-8: https://pep8.org/#function-names
Edit: There's a similar case in the function below already so I guess this is fine.
There was a problem hiding this comment.
Yeah, PEP 8 is a set of guidelines to encourage consistency and readability, not strict rules that Guido wants you to follow mindlessly :-). (He actually got frustrated at the pep8 tool and asked its devs to rename it to "pycodestyle", because he didn't think PEP 8 can be enforced by an automatic tool.) Here, no one is going to get confused by the name into thinking it's anything besides a function, and the name is descriptive, so... works for me.
|
I added sphinx markup to the newsfragment (and tweaked the wording a little bit). |
|
Okay merged! 🎉 🎂 And, no pressure, but if you'd like to keep contributing then we'd love to have you, so I'm sending you a github invite now. You can read more about this in our contributing documentation. |
|
Thanks! Happy to be a member of the trio team :-)
…On Sun, Oct 28, 2018 at 8:01 PM Nathaniel J. Smith ***@***.***> wrote:
Okay merged! 🎉 🎂 And, no pressure, but if you'd like to keep
contributing then we'd love to have you, so I'm sending you a github invite
now. You can read more about this in our contributing documentation
<https://trio.readthedocs.io/en/latest/contributing.html#joining-the-team>
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#728 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAS5e5ORYT4uO4OK4hQfYkXd9o3tKhdrks5upkVGgaJpZM4XRjHv>
.
|
Looking to make my first (small) contribution to trio with a "good first issue" (#618)