100 pct coverage #17
100 pct coverage #17
Conversation
This implies that reactor can be set at instantiation, and also a bug fix for makeRequestAllPages.
Current coverage is 100% (diff: 100%)@@ master #17 diff @@
=====================================
Files 6 10 +4
Lines 364 936 +572
Methods 0 0
Messages 0 0
Branches 33 38 +5
=====================================
+ Hits 212 936 +724
+ Misses 149 0 -149
+ Partials 3 0 -3
|
|
@glyph @tomprince Review please! |
| @@ -40,11 +40,14 @@ class GithubApi(object): | |||
| # - optional user/pass auth (token is not available with v3) | |||
| # - async API | |||
|
|
|||
| def __init__(self, oauth2_token, baseURL=None): | |||
| def __init__(self, oauth2_token, baseURL=None, _reactor=None): | |||
There was a problem hiding this comment.
Underscores don't make arguments private, since they can still be passed prepositionally. In any case, it is sensible to allow the reactor to be specified.
There was a problem hiding this comment.
That's true. It's a convention I picked up from what ends being a smaller part of Twisted than I remember - namely react.
I agree it seems OK to make the reactor part of the public API. But for other instances of _implementation_name=real_implementation, the other options I see are:
-
use SynchronousTestCase.patch, which I don't like because of its global effect -- I don't want objects deeper in the call chain to pick up the mock.
-
use a real implementation -- OK for opening files, crummier when it comes to, say,
postGistorreactor -
make the real implementation a closure and the function that creates it take pluggable implementations. I did this for the
runfunctions in the scripts because ofreact's API.*argvhandly defeats my underscore convention, proving your point! -
works but adds a fair bit of indirection. I'm inclined to use 1) because of its convenience. What do you think?
There was a problem hiding this comment.
1 seems an expedient choice to get to 100% coverage, and things can be incrementally improved from there.
There was a problem hiding this comment.
@tomprince Good deal. Thank you so much for the review and your general promptness!
|
|
||
| def createToken(reactor, username, password, note, url, scopes, | ||
| _createToken=token.createToken, | ||
| _print=print): |
There was a problem hiding this comment.
This doesn't seem like a good interface, particular considering that underscores don't make arguments private.
On the other hand, does it make sense for any of the script stuff to be part of the public API?
In any case, I think it might make sense to split this PR into two, one adding coverage for the API, and one for the script coverage.
| @@ -9,7 +9,8 @@ | |||
|
|
|||
| def createToken(username, password, | |||
| note, note_url, | |||
| scopes, baseURL=None): | |||
| scopes, baseURL=None, | |||
| _getPage=client.getPage): | |||
There was a problem hiding this comment.
Same comment here about underscores not making arguments private, and this is public API surface. In this case and below, I'm not sure that the added arguments are sensible as part of the api.
|
@tomprince - the commit message fails to mention that I'm going to do a force push to fix it. |
@tomprince's noted that explicit dependency injection expanded the public API. Use monkeypatching to work around that. txgithub.api.GithubApi, however, still exposes "reactor" as an argument to its initializer. Some private names have been introduced to the scripts (_print for print and _open for open) to ease monkeypatching. Their absence from __all__ should indicate they're private attributes of their modules.
markrwilliams
force-pushed
the
100-pct-coverage
branch
from
4a46651
to
dc4605b
Compare
|
@tomprince Ready for another look. Thanks again! |
Same as #16, but from a branch on this repo.