Add `--use-colors` / `--no-use-colors` flags. #502

euri10 · 2019-11-30T16:48:17Z

A potential attempt at #497

I wish I could have found a way to add tests but I have no idea as to what to test, I can see if I pass the use_colors flag to False that it's working but that's weak

True

False

…olourizedFormatter

tomchristie · 2019-12-03T12:10:56Z

uvicorn/config.py

        log_level=None,
        access_log=True,
+        use_colors=True,


We should default this to None

tomchristie · 2019-12-03T12:11:12Z

uvicorn/config.py

@@ -57,12 +57,12 @@
 SSL_PROTOCOL_VERSION = getattr(ssl, "PROTOCOL_TLS", ssl.PROTOCOL_SSLv23)


-LOGGING_CONFIG = {
+LOGGING_CONFIG_COLORS = {


Let's just stick with a single LOGGING_CONFIG.

We can pass an argument "use_colors": None, in the config to the DefaultFormatter, to indicate that it should be autodetermined.

If use_colors=[True|False] is pass to the config, then we should update just that bit in the config dictionary.

tomchristie · 2019-12-03T12:15:07Z

uvicorn/config.py

@@ -215,10 +250,15 @@ def __reduce__(self):
            logging.Logger.__reduce__ = __reduce__

        if self.log_config is not None:
+            if not self.use_colors:
+                self.log_config = LOGGING_CONFIG_NO_COLORS


Instead of switching the entire config here, lets use...

if self.use_colors in (True, False): self.log_config["formatters"]["default"]["use_colors"] = self.use_colors

We should only do this within the if isinstance(self.log_config, dict): case. If someone is using the file config then it's up to them to choose the correct settings there.

added the same logic for access too

tomchristie

Great stuff!

I've got a bit of a requested tweak in how in think we should handle this... let me know if I've not explained something clearly enough here!

…ors to None

euri10 · 2019-12-03T13:14:05Z

Great stuff!

I've got a bit of a requested tweak in how in think we should handle this... let me know if I've not explained something clearly enough here!

hopefully I got this right ;)

tomchristie · 2019-12-03T15:52:59Z

uvicorn/config.py

                logging.config.dictConfig(self.log_config)
            else:
                logging.config.fileConfig(self.log_config)
+        else:
+            pass


Doesn't look like we need the else: pass block here.

tomchristie · 2019-12-03T15:54:51Z

uvicorn/logging.py

-    def __init__(self, fmt=None, datefmt=None, style="%"):
-        self.use_colors = self.should_use_colors()
+    def __init__(self, fmt=None, datefmt=None, style="%", use_colors=None):
+        self.use_colors = use_colors


We'd want to do...

if use_colors in (True, False): self.use_colors = use_colours else: self.use_colors = sys.stdout.isatty()

tomchristie

Yes, great stuff!
Just a coupla minor comments to address.

tomchristie · 2019-12-03T17:06:14Z

Fantastic!
Last bit is the docs...

The homepage and deployment pages both include the output of uvicorn --help which will need to be updated.
The settings page includes a "Logging" section which is probably where we want to add your flag docs into.

euri10 · 2019-12-03T17:09:49Z

Fantastic!
Last bit is the docs...

The homepage and deployment pages both include the output of uvicorn --help which will need to be updated.

The settings page includes a "Logging" section which is probably where we want to add your flag docs into.

yes sorry I forgot the docs, will do that asap

euri10 · 2019-12-04T07:15:38Z

just one thing, I modified the cli flag too as it was wrong (it was "--use-colors/--use-colors" and I realized it pasting the --help output), I have one question about the default I set to None, but I'm not too sure it's valid in case of a bool

also I'm not sure but think the huge diff in the uvicorn --help output comes from the fact it has not been updated in a long time, and for the index.md it seems previous version had not the 2 spaces before each flag which explains the huge diff

euri10 · 2019-12-04T10:27:01Z

added some examples of custom logging config for #491
I realized during this docs aren't tested, might be worth it

edit: commit message wanted to link 491 not 409

tomchristie · 2019-12-04T10:37:24Z

added some examples of custom logging config

Okay - Let's drop those out of this pull request. We may want a PR for them, but that's a seperate issue, and I'd prefer to review it in isolation to the rest of this work.

euri10 · 2019-12-04T11:43:25Z

added some examples of custom logging config

Okay - Let's drop those out of this pull request. We may want a PR for them, but that's a seperate issue, and I'd prefer to review it in isolation to the rest of this work.

ok I'm no git pro but I think this might be ok, I did
git branch logging_examples
git reset --keep HEAD~1
git push -f origin

so now colors branch should be in the state it was before adding the examples,

tomchristie · 2019-12-04T11:59:16Z

docs/deployment.md

+  --log-level [critical|error|warning|info|debug|trace]
+                                  Log level. [default: info]
+  --access-log / --no-access-log  Enable/Disable access log.
+  --use-colors / --use-colors     Enable/Disable colorized logging.


Looks like this should be --use-colors / --no-use-colors

ok I was sure I pasted it....now I'm sure

tomchristie · 2019-12-04T11:59:39Z

docs/index.md

+  --log-level [critical|error|warning|info|debug|trace]
+                                  Log level. [default: info]
+  --access-log / --no-access-log  Enable/Disable access log.
+  --use-colors / --use-colors     Enable/Disable colorized logging.


Presumably --use-colors / --no-use-colors, right?

tomchristie · 2019-12-04T12:00:41Z

docs/settings.md

@@ -30,6 +30,7 @@ equivalent keyword arguments, eg. `uvicorn.run("example:app", port=5000, reload=
 * `--log-config <path>` - Logging configuration file.
 * `--log-level <str>` - Set the log level. **Options:** *'critical', 'error', 'warning', 'info', 'debug', 'trace'.* **Default:** *'info'*.
 * `--no-access-log` - Disable access log only, without changing log level.
+* `--use-colors` - Enable colorized formatting of the log records.


We should change this doc around so that:

We include both the "on" and "off" flag variants.

It's clear that we'll autodetect if the flag isn't used.

changed, let me know if this is better

tomchristie · 2019-12-04T12:03:34Z

"I'm no git pro but I think this might be ok, I did"

Looks okay to me, yup.

As it happens you don't actually need to do those smarts - we have a policy of squashing changes down into a single-commit-per-pull-request on merges, so we don't get any of the messy "getting towards the goal" individual commits, but instead just have a single "here's the PR functionality" commit in the history.

You'd be absolutely fine just making a new commit that removed the work we didn't want in here.

tomchristie · 2019-12-09T12:22:09Z

Thanks for this!

euri10 · 2019-12-10T20:17:58Z

I think I somewhat f*** up this PR by fogetting to add the new flag in the main() @tomchristie

tthe below patch should fix it I think, not sure how to proceed from here

Index: uvicorn/main.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- uvicorn/main.py	(revision abd4b006c8a443bc80201409187ab3a662ac7c7e)
+++ uvicorn/main.py	(date 1576008880785)
@@ -257,6 +257,7 @@
     ssl_ca_certs: str,
     ssl_ciphers: str,
     headers: typing.List[str],
+    use_colors: bool,
 ):
     sys.path.insert(0, ".")
 
@@ -292,6 +293,7 @@
         "ssl_ca_certs": ssl_ca_certs,
         "ssl_ciphers": ssl_ciphers,
         "headers": list([header.split(":") for header in headers]),
+        "use_colors": use_colors,
     }
     run(**kwargs)

tomchristie · 2019-12-10T21:48:00Z

the below patch should fix it I think

Looks right to me too. Wanna issue a PR for that?

euri10 · 2019-12-10T21:54:05Z

Will do for sure tomorrow! Le mar. 10 déc. 2019 à 10:48 PM, Tom Christie <[email protected]> a écrit :

…

the below patch should fix it I think Looks right to me too. Wanna issue a PR for that? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#502?email_source=notifications&email_token=AAINSPSMOX5OQLAEV2FPW2DQYAFBDA5CNFSM4JTHMIRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGRCG6I#issuecomment-564274041>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAINSPSRQGT5MFVYVZCY6M3QYAFBDANCNFSM4JTHMIRA> .

euri10 · 2019-12-11T06:49:16Z

ok done in #520

euri10 added 4 commits November 30, 2019 17:22

Changed default log_config to LOGGING_CONFIG_COLORS and make it use C…

9883c7a

…olourizedFormatter

Removed tty detection and set color based on use_color flag

50b2f7e

Added cli flag

502217e

Black and autoflake were forgotten

7e3c42b

tomchristie reviewed Dec 3, 2019

View reviewed changes

tomchristie requested changes Dec 3, 2019

View reviewed changes

euri10 added 2 commits December 3, 2019 14:11

Reverted to a single LOGGING_CONFIG with DefaultFormatter and use_col…

23f0b88

…ors to None

Format

8cc6ddc

tomchristie reviewed Dec 3, 2019

View reviewed changes

tomchristie requested changes Dec 3, 2019

View reviewed changes

Reworked tty detection

14d31eb

euri10 added 5 commits December 4, 2019 08:03

Updates uvicorn --help in docs

538e765

Updates uvicorn --help in index.md too

ee7c580

Modified flag and its default

a43ab16

Update cli help given updated flag

3a30b94

Added settings

160b8db

euri10 force-pushed the colors branch from 8844943 to 160b8db Compare December 4, 2019 11:40

tomchristie reviewed Dec 4, 2019

View reviewed changes

euri10 added 2 commits December 4, 2019 13:32

Fixed flag

66dc64a

Precsions on flags meaning and default behaviour

a3d65af

tomchristie changed the title ~~should use colors~~ Add --use-colors / --no-use-colors flags. Dec 9, 2019

tomchristie merged commit be50ca6 into encode:master Dec 9, 2019

tomchristie mentioned this pull request Dec 9, 2019

should_use_colors inside debugger #497

Closed

euri10 deleted the colors branch December 9, 2019 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--use-colors` / `--no-use-colors` flags. #502

Add `--use-colors` / `--no-use-colors` flags. #502

euri10 commented Nov 30, 2019

tomchristie Dec 3, 2019

euri10 Dec 3, 2019

tomchristie Dec 3, 2019 •

edited

Loading

euri10 Dec 3, 2019

tomchristie Dec 3, 2019

euri10 Dec 3, 2019

tomchristie left a comment

euri10 commented Dec 3, 2019

tomchristie Dec 3, 2019

tomchristie Dec 3, 2019

tomchristie left a comment

tomchristie commented Dec 3, 2019

euri10 commented Dec 3, 2019

euri10 commented Dec 4, 2019 •

edited

Loading

euri10 commented Dec 4, 2019 •

edited

Loading

tomchristie commented Dec 4, 2019

euri10 commented Dec 4, 2019

tomchristie Dec 4, 2019

euri10 Dec 4, 2019

tomchristie Dec 4, 2019

euri10 Dec 4, 2019

tomchristie Dec 4, 2019

euri10 Dec 4, 2019

tomchristie commented Dec 4, 2019

tomchristie commented Dec 9, 2019

euri10 commented Dec 10, 2019

tomchristie commented Dec 10, 2019

euri10 commented Dec 10, 2019 via email

euri10 commented Dec 11, 2019

Add --use-colors / --no-use-colors flags. #502

Add --use-colors / --no-use-colors flags. #502

Conversation

euri10 commented Nov 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchristie Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchristie left a comment

Choose a reason for hiding this comment

euri10 commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchristie left a comment

Choose a reason for hiding this comment

tomchristie commented Dec 3, 2019

euri10 commented Dec 3, 2019

euri10 commented Dec 4, 2019 • edited Loading

euri10 commented Dec 4, 2019 • edited Loading

tomchristie commented Dec 4, 2019

euri10 commented Dec 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchristie commented Dec 4, 2019

tomchristie commented Dec 9, 2019

euri10 commented Dec 10, 2019

tomchristie commented Dec 10, 2019

euri10 commented Dec 10, 2019 via email

euri10 commented Dec 11, 2019

Add `--use-colors` / `--no-use-colors` flags. #502

Add `--use-colors` / `--no-use-colors` flags. #502

tomchristie Dec 3, 2019 •

edited

Loading

euri10 commented Dec 4, 2019 •

edited

Loading

euri10 commented Dec 4, 2019 •

edited

Loading