Code

Opened 4 years ago

Last modified 4 years ago

#13291 new New feature

Allow `color_style()` and `parse_color_setting()` to be used with custom colour palettes.

Reported by: mrmachine Owned by: nobody
Component: Core (Management commands) Version: master
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)

13291-custom-color-palettes-r12937.diff (7.0 KB) - added by mrmachine 4 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 4 years ago by mrmachine

  • Component changed from Uncategorized to django-admin.py
  • Keywords management command customize color roles added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 1.1 to SVN

Changed 4 years ago by mrmachine

comment:2 Changed 4 years ago by mrmachine

  • Has patch set

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.

comment:3 Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 4 years ago by carljm

  • 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 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to New feature

comment:6 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:7 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.