Fixed #29026 -- Added --scriptable option to makemigrations.

[jacobtylerwalls]

ticket-29026 desires to separate logging from a simple list of filepaths created in makemigrations.

• Today, there is logging to both stdout and stderr (for errors), but no real "program output": the filenames are bolded and indented to flow between other log messages.
• This PR creates a new option --scriptable that will 1. divert all current logging to stderr and 2. log only the filepaths of created migration files to stdout, one per line (without leading spaces and styling)
• --noinput mode is still necessary to suppress input completely, otherwise interactive prompts go to stderr when --scriptable is used

EDIT: moved ticket-29470 solution to #14805

All of this logging can be silenced with --verbosity 0: no changes in this respect.

[cjerdonek]

The direction we're going elsewhere is to make log() a method that accepts a msg argument. See e.g.

django/django/db/backends/base/creation.py

Lines 29 to 30 in 8208381

	def log(self, msg):
	sys.stderr.write(msg + os.linesep)

and

django/django/test/runner.py

Lines 672 to 683 in 8208381

	def log(self, msg, level=None):
	"""
	Log the given message at the given logging level.
	A verbosity of 1 logs INFO (the default level) or above, and verbosity
	2 or higher logs all levels.
	"""
	if self.verbosity <= 0 or (
	self.verbosity == 1 and level is not None and level < logging.INFO
	):
	return
	print(msg)

My thoughts would be to add an initial commit that adds a log() method and makes all stderr / stdout output go through that. (Whether log() uses stderr or stdout could be controlled centrally in that method.) Then, rather than structured_output, I would add a property whose job is to return whether log() is using stderr. If it's using stderr, then the "output" code lines could switch to using stdout instead of log(). The thing with structured is that text lines aren't that structured. To me that sounds more like json. It seems like the user-facing option should be more about whether logging should go to stderr. Then the non-logging ("output") lines would continue to go to stdout as is.

Also, in the case that logging is going to stderr, I think you'd still want to log the output lines using log() as a message, so that info would be available both in the diagnostic stderr logs, as well as the output.

[jacobtylerwalls]

The thing with structured is that text lines aren't that structured. To me that sounds more like json.

Very reasonable--I'll think about a name for this option.

It seems like the user-facing option should be more about whether logging should go to stderr. Then the non-logging ("output") lines would continue to go to stdout as is.

My thought process was this: merely switching the output to stderr is achievable today: you simply subclass the command and set self.stdout to whatever you want, sys.stderr, or whatever. I'm not sure we would add complexity if that were the only thing we were achieving. I think the reason for this PR is that we don't have any "non-logging/output" lines today, since this is not really nonlogging (leading spaces, hyphen, and colors bolding):

 \x1b[1m/var/folders/6q/jljmh5xs27v8_7557q3rgtfw0000gn/T/django_klnxitjw/tmpwiefzufk/tmp82ptgwlr/migrations/0001_initial.py\x1b[0m\n

My thoughts would be to add an initial commit that adds a log() method and makes all stderr / stdout output go through that.

So I'm a little hesitant to do to this, because then we're making makemigrations more special than the other commands. This ticket is asking to make makemigrations more special by producing different kinds of output, so I guess we're already starting down that path.

Also, in the case that logging is going to stderr, I think you'd still want to log the output lines using log() as a message, so that info would be available both in the diagnostic stderr logs, as well as the output.

Agreed, I made sure this was the case.

---

Any thoughts on the ticket-29470 stuff? If we have design questions about ticket-29026 I could ask the fellows to re-triage 29470 and move it to a separate PR to keep it moving.

[jacobtylerwalls]

btw, thanks for the review, @cjerdonek! And it looks like I need to liberalize one of the tests to pass on Windows.

[jacobtylerwalls]

Maybe --separatelogs?

Everything but the last line going to stderr, last line to stdout:

Migrations for 'migrations':
  /var/folders/6q/jljmh5xs27v8_7557q3rgtfw0000gn/T/django_e_o35bow/tmpso0994dn/tmp7ogni0kc/migrations/0001_initial.py
    - Create model ModelWithCustomBase
    - Create model SillyModel
    - Create model UnmigratedModel
/var/folders/6q/jljmh5xs27v8_7557q3rgtfw0000gn/T/django_e_o35bow/tmpso0994dn/tmp7ogni0kc/migrations/0001_initial.py

[jacobtylerwalls]

you simply subclass the command

This didn't feel right when I wrote it, and indeed--it's easier than that, there's call_command(stdout=), see: https://docs.djangoproject.com/en/3.2/ref/django-admin/#output-redirection

The drift of my earlier comment was that I would be hesitant to rework the documented API we have for that. I see the issue as more defining wanting separate logs, period.

[cjerdonek]

To respond to one point now:

My thoughts would be to add an initial commit that adds a log() method and makes all stderr / stdout output go through that.

So I'm a little hesitant to do to this, because then we're making makemigrations more special than the other commands.

I think making log() a method that is passed a message is more natural and has advantages over making it an attribute with a write() method. For example, it would give people a way to use a Python logger instead of writing to a stream. This is the approach taken in this commit for ticket #14150. Also, the word "log" is more commonly used in Python as the verb / method name rather than the stream (see e.g. Logger.log() in Python's logging module). It would also make the calling sites simpler / cleaner. Finally, if this pattern is found useful, it could be moved to BaseCommand so makemigrations.py wouldn't be so special.

[jacobtylerwalls]

Also, the word "log" is more commonly used in Python as the verb / method name rather than the stream (see e.g. Logger.log() in Python's logging module).

I have to admit, this did occur to me when I was writing it, and if you also noticed it, lots of folks will. :-O

For example, it would give people a way to use a Python logger instead of writing to a stream. This is the approach taken in this commit for ticket #14150.

That's a good reason.

Finally, if this pattern is found useful, it could be moved to BaseCommand so makemigrations.py wouldn't be so special.

I guess I wasn't looking at it that way. 👍🏻 Thanks for the quick feedback, and I'll have a look at implementing a log() method.

[cjerdonek]

Maybe --separatelogs?

Maybe --scriptable or --scriptmode? The option seems like a higher-level mode as it does two things: it changes logging to go to stderr, and it adds additional info to stdout.

[jacobtylerwalls]

Maybe --scriptable or --scriptmode?

At a glance, I would be a bit worried it implies setting --noinput can be skipped when using in a script.

[cjerdonek]

At a glance, I would be a bit worried it implies setting --noinput can be skipped when using in a script.

I think there's an argument the mode should imply --noinput. The reason is that, when programmatically consuming the stdout (e.g. piping it to a file), you wouldn't see the prompt anyways. (It would show up to the user as a hang.) This is because the command's questioner uses Python's input() built-in, which writes to stdout. So it seems incompatible with consuming stdout for programmatic use. Maybe you could write a prompt to stderr, but that seems non-standard.

Either way, I think you'd want to document in the help whether --noinput is implied, which should eliminate any worries.

[cjerdonek]

Reading and thinking more about Python's input() and how --noinput behaves, I think the scriptable mode option we're discussing in this PR shouldn't default to --noinput, and when it's used, the questioner should write its prompt to stderr instead of stdout (and not use log()). There are a couple reasons for the latter. First, there is a very old Python ticket to change input() to use stderr, so it wouldn't actually be non-standard to use stderr like I suggested in my comment above. Secondly, if the mode didn't use stderr, then there would be no way to use scriptable mode while capturing the output stream (one of its intended use cases) when answers are required that differ from the default answers when --noinput is used. Lastly, I said "not use log()" because if someone, say, changed log() to log to a file, you would still want the prompt to go to stderr so the user could provide interactive feedback.

Also, on this question:

Any thoughts on the ticket-29470 stuff?

After reading and thinking about it, I think it should be re-opened and handled separately. I can go ahead and add a comment to the ticket.

[jacobtylerwalls]

Hello @francoisfreitag -- I noticed you have a WIP branch for ticket-21429 implementing logging for each of the management commands. As you can see above, Chris is making the good case that to move this PR forward, I should do something similar for makemigrations. Are you still interested in contributing your work? If #13853 is the only blocker, we can see about getting it into the review queue. Let me know if I can be helpful in any respect. 👍🏻

[cjerdonek]

@jacobtylerwalls By the way, regarding the log() method I was suggesting you add above, the collectstatic management command is another class that already has a method like that:

django/django/contrib/staticfiles/management/commands/collectstatic.py

Lines 207 to 212 in 8208381

	def log(self, msg, level=2):
	"""
	Small log helper
	"""
	if self.verbosity >= level:
	self.stdout.write(msg)

It will be of help here independent of that ticket.

[francoisfreitag]

Hi @cjerdonek,

Thanks for the ping! Fixing ticket-21429 is pretty time consuming and personal life has been busy (and will be for at least a few months). My employer is not interested in sponsoring the work for now, so it’s all on my personal time.

Basically, my todo list includes:

• rebasing on main (last rebase was probably 9 months ago)
• a readthrough, making sure:
  • assertNoLogs and assertLogRecords are used where possible
  • reviewing uses of io, StringIO, stdout and stderr in test code
  • code polish (e.g. grouping context managers, preferring single quotes, f-strings, etc)
• consider introducing flake8-logging-format
• installing my branch on an existing project shows unexpected line returns if I don’t change the existing project config. Needs investigation.
• Completing the documentation
• Testing against all DBs. I tried to use exact assertions for the logging output as much as possible, but different DB engines or configuration may cause messages to change slightly.

#13853 is just a very early step, introducing tools I would like to use for the logging PR. I’m afraid there are weeks of work remaining before my branch can be put up for review, and I am not able to commit to a timeline.

[jacobtylerwalls]

Thanks for the update. I certainly wouldn't ask you to commit to a timeline! I was just merely curious if I should try to conform to any likely pending changes. I think I have enough to go on for now. Good luck with everything, and be well. --Jacob

[jacobtylerwalls]

Thanks for the design guidance @cjerdonek , that was very helpful. I haven't squashed/reordered commits yet, and I am still thinking about #14751 (comment), but I wanted to ask if you thought these changes were looking right-track to you. I'd be grateful if you had time for a re-review.

I didn't as of yet tackle a refactor of how verbosity is tracked. There's enough going on (and I feel like this PR is already two or so.) Speaking of which, thanks for commenting on ticket-29470.

[cjerdonek]

Hi @jacobtylerwalls, yes, I think there's already a lot going on, so I'd encourage you to start breaking this up into smaller PR's. The first one I'd recommend is a refactoring PR that just adds a log() method, and in particular doesn't make any reference to scriptable, etc. The next step PR can be discussed after that.

[jacobtylerwalls]

The first one I'd recommend is a refactoring PR that just adds a log() method, and in particular doesn't make any reference to scriptable, etc. The next step PR can be discussed after that.

Done! 🎉
#14936

[jacobtylerwalls]

Summary:

Adds --scriptable flag to makemigrations, causing:

• log() method to divert output to self.stderr (agreement on this here: "whether log() uses stderr or stdout could be controlled centrally in that method")
• Interactive questioner to divert prompts to self.stderr (agreement here)
• an additional line of "clean output" (no styling or indentation) written to self.stdout (original case from ticket)

That's all the diff does, it's just bloated because the interactive questioner was using print() statements, which had to be rewritten. I can move that to another commit (or PR?) if folks like.

Could also be good to rebase after #15212.

[felixxm]

@jacobtylerwalls Thanks 👍 Can we move adding prompt_output to the InteractiveMigrationQuestioner to a separate PR?

[felixxm]

@jacobtylerwalls Thanks 👍

I added writing a path of generated migration file to the --merge option (with tests) and pushed small edits.

[jacobtylerwalls]

@felixxm Thanks for the updates and the additional test!