Opened 15 years ago
Last modified 14 years ago
#13291 new New feature
Allow `color_style()` and `parse_color_setting()` to be used with custom colour palettes.
Reported by: | Tai Lee | Owned by: | nobody |
---|---|---|---|
Component: | Core (Management commands) | Version: | dev |
Severity: | Normal | Keywords: | management command customize color roles |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
I want to add new roles and change the default terminal colours that ship with Django, for use in my own project's management commands, but I want to retain the ability for these to be overridden with a DJANGO_COLORS
environment variable. I also don't want to repeat myself by copying the entire color_style()
and parse_color_setting()
functions, which are hard coded to use django.utils.termcolor.PALETTES
and which ignore any roles specified in DJANGO_COLORS
which are not also present in django.utils.termcolors.PALETTES[NOCOLOR_PALETTE]
.
It turns out that it's fairly trivial to add a palettes
argument to both functions, which will allow me to simply define my own dictionary of palettes somewhere (or import and extend the ones that ship with Django) and then call color_style(my_palettes)
to get a style object with roles and default colours that are appropriate for my project, and also retain the ability for users to customise the colours for my roles with a DJANGO_COLORS
environment variable.
Attachments (1)
Change History (8)
comment:1 by , 15 years ago
Component: | Uncategorized → django-admin.py |
---|---|
Keywords: | management command customize color roles added |
Version: | 1.1 → SVN |
by , 15 years ago
Attachment: | 13291-custom-color-palettes-r12937.diff added |
---|
comment:2 by , 15 years ago
Has patch: | set |
---|
comment:3 by , 15 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 14 years ago
Patch needs improvement: | set |
---|
Some notes on the patch:
I think the documentation addition is misplaced; rather than being in the django-admin.py documentation, it should go in the "writing custom management commands" howto, as it's only relevant to someone who's writing a custom command, not all users of django-admin.py.
I think it also needs more context; there currently is no documentation at all for using django's style/colors system in your own custom management command. If we're going to bother adding the customizability this patch offers, we should document it, and that means full documentation that would actually allow someone to use it, including how to actually do the styled output, etc.
Also, as currently written, the docs show updating PALETTES directly, which monkeypatches the built-in palettes. Actually, even the .copy() method used in the tests, presumably to avoid that, is not effective: only a shallow copy of the outer dictionary is performed, so the inner palette dictionaries are still referring to the same actual dictionary, and updating them _still_ in fact monkeypatches the built-in palettes.
All of which leads me in a roundabout way to my only issue with the code patch itself: it suggests a usage pattern that is too easy to misuse, as demonstrated by the fact that both the provided test and the provided docs misuse it! I think perhaps a better option for the API would be to pass in palette_overrides rather than completely new palettes; this would make the usage look more like palette_overrides={'dark': {'WARNING': {'fg': 'yellow'}}}. Or, perhaps even better, to reuse the more compact DJANGO_COLORS string syntax, so you would just pass in palette_overrides="warning=yellow".
comment:5 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
Patch with tests and docs attached. Also fixed a related bug when calling
style.ROLE(integer)
which tried to concatonate the integer with a string, rather than use string formatting to convert the integer to string.