Code

Opened 3 years ago

Closed 5 months 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: master
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)

use-self.diff (9.8 KB) - added by adamv 3 years ago.
Use self.stderr|out instead of print or sys.
15107-2.diff (71.0 KB) - added by claudep 2 years ago.
Commands output through logging
15107-3.diff (70.4 KB) - added by claudep 2 years ago.
Updated to current trunk

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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.

Changed 3 years ago by adamv

Use self.stderr|out instead of print or sys.

comment:2 Changed 3 years ago by adamv

  • Has patch set

comment:3 Changed 3 years ago by jezdez

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.

Version 0, edited 3 years ago by jezdez (next)

comment:4 Changed 3 years ago by adamv

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

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'.

Last edited 3 years ago by jezdez (previous) (diff)

comment:6 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:7 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Changed 2 years ago by claudep

Commands output through logging

comment:8 Changed 2 years ago by claudep

  • 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 Changed 2 years ago by claudep

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!)

Changed 2 years ago by claudep

Updated to current trunk

comment:10 Changed 2 years ago by Mike <leachim@…>

@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 Changed 2 years ago by claudep

  • 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 Changed 5 months ago by claudep

  • Resolution set to fixed
  • Status changed from new to closed

Let's close this one in favour of #21429

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.