Opened 14 years ago
Closed 11 years ago
#15107 closed Cleanup/optimization (fixed)
Convert core commands to use self.std(out|err) instead of sys.std(out|err)/print
Reported by: | mmcnickle | Owned by: | mmcnickle |
---|---|---|---|
Component: | Core (Management commands) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Some core management commands are using raw "print" statements and calls to sys.stdout/sys.stderr instead of self.stdout/self.stderr. This causes an inconsistency between commands when people specify their own buffers when using django.core.management.call_command:
out = StringIO() err = StringIO() call_command('command', stdout=out, stderr=err) ...
Attachments (3)
Change History (15)
comment:1 by , 14 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 14 years ago
Has patch: | set |
---|
comment:3 by , 14 years ago
I would rather like to see the logging module be used here, if possible, as we've seen a tremendous slowdown when we've ported the staticfiles app to django and removed the logging from commands.
comment:4 by , 14 years ago
Patch needs improvement: | set |
---|
@jezdez If you point me to an example where logging is used "the right way", I'll try to work up a patch.
comment:5 by , 14 years ago
In django-staticfiles we had an own management command specific logger mixin, https://github.com/jezdez/django-staticfiles/blob/0.3.4/staticfiles/management/base.py but I admit that's a bit cumbersome given our new logging API in 1.3. I'd like to propose to add an additional logger (next to the 'django'
, 'django.request'
and 'django.db.backends'
loggers (http://docs.djangoproject.com/en/dev/topics/logging/#id1), e.g. 'django.managment.commands'
or 'django.commands'
.
comment:6 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Cleanup/optimization |
comment:8 by , 13 years ago
Easy pickings: | unset |
---|---|
Patch needs improvement: | unset |
UI/UX: | unset |
I worked on a refactor of commands output using logging infrastructure, adding new BaseCommand.info() and BaseCommand.error() proxy methods, and deprecating self.stderr and self.stdout. Hopefully this is in the right direction, as of jezdez comment:5.
comment:9 by , 13 years ago
Just a note to any potential reviewer: as this patch is quite extensive, I will keep a local branch updated with current trunk. However, I will not spam this ticket each time I rebase my branch. So just ping me if/when you are interested to review it (hopefully during the 1.5 timeframe!)
comment:10 by , 13 years ago
@claudep
If BaseCommand has .info() and .error(), it might be nice to be complete and include .debug(), .warning() and .critical(), that also get output to the stdout/stderr passed to the command.
Then the verbosity argument could potentially be updated to also take strings, so you could for example do -v W to only have output at warning level or higher.
comment:11 by , 13 years ago
Patch needs improvement: | set |
---|
Thanks Mike, that's an interesting comment. I'd like to push patch for #18325 first, then rewrite this one with your idea in mind.
comment:12 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Let's close this one in favour of #21429
For background -- the transition to self.stdout/stderr was done to ensure easy testing, and as a side effect, to allow Django commands to be used as a service. We've converted those commands that needed to be converted for the purpose of Django's own testing; the remainder is left as a work in progress.