run_app: make print=None disable printing #2260
Conversation
|
+1: for this, but I've always thought the Really this should be logged, but since python (very unfortunately) python doesn't have a Perhaps (I'm guess you had to create the PR before you could add a file to |
seirl
force-pushed
the
master
branch
2 times, most recently
from
15cfdb6 to
e96ed58
Compare
|
2017-09-11 12:39 GMT+02:00 Samuel Colvin <[email protected]>:
+1: for this, but I've always thought the print argument was ugly.
Really this should be logged, but since python (very unfortunately) python doesn't have a NOTICE log level I'm not sure of the proper solution.
Perhaps aiohttp should have a log which is setup to write to stdout out at INFO level by default?
I agree with all of that, and probably a verbose=True argument instead
of print=. That said, I did not feel confident to add logging since
aiohttp doesn't use much logging at all, and since this would have
been a backwards incompatible change I'd rather have this quick and
easy solution in first before we find a "proper" way to replace it.
(I'm guess you had to create the PR before you could add a file to changes?)
Yeah, sorry, it was in my staged changes but I went to lunch and forgot.
It's there now.
|
|
The lambda assignment made QA explode so I'm doing a regular |
|
I'm ok with PR but disagree with logging usage for printing text like |
aiohttp/web.py
Outdated
| @@ -422,6 +422,10 @@ def run_app(app, *, host=None, port=None, path=None, sock=None, | |||
| app._set_loop(loop) | |||
| loop.run_until_complete(app.startup()) | |||
|
|
|||
| if print is None: | |||
There was a problem hiding this comment.
much easier just do if print below before calling it.
There was a problem hiding this comment.
I guess, if that's the only place you ever plan to use that print argument?
There was a problem hiding this comment.
looks like you having a linting error 😄
There was a problem hiding this comment.
Woops, failed my block selection. There you go, sorry :-P
seirl
force-pushed
the
master
branch
2 times, most recently
from
d5dfb62 to
7b20128
Compare
Codecov Report
@@ Coverage Diff @@
## master #2260 +/- ##
==========================================
+ Coverage 97.31% 97.31% +<.01%
==========================================
Files 39 39
Lines 7944 7945 +1
Branches 1377 1378 +1
==========================================
+ Hits 7731 7732 +1
Misses 90 90
Partials 123 123
Continue to review full report at Codecov.
|
|
... wtf? |
|
codecov occasionally goes mad and does strange things. Is the branch bases off current master? |
|
Sure, parent commit is c7e8356 . |
|
Would probably be good to add a test for |
|
Okay, rather than adding a new test I updated one that already contained |
| @@ -447,8 +447,9 @@ def run_app(app, *, host=None, port=None, path=None, sock=None, | |||
| pass | |||
|
|
|||
| try: | |||
| print("======== Running on {} ========\n" | |||
| "(Press CTRL+C to quit)".format(', '.join(uris))) | |||
| if print: | |||
There was a problem hiding this comment.
if callable(print): maybe? The discoverability of this argument is poor, intuition makes me want to use print=True which will fail miserably.
There was a problem hiding this comment.
better to "fail miserably" than fail silently. In neither case would print=True do what you expected.
Much better to use python typing hinting for public methods like this once 3.4 support is dropped.
|
Thanks! |
psf-chronographer
bot
added
the
bot:chronographer:provided
run_app currently takes a callable compatible with
print(). If you want to disable output completely (which happens all the time when you're writing test suites or similar), you have to call run_app withprint=lambda *_, **__: None, which is not very pretty and confusing.This behavior is annoying, so this change makes print=None a special case that disables output completely.
This change is backwards-compatible.