Fixed #27600 -- Improved the way the 'shell' command reads python code from stdin. #7681

jpic · 2016-12-13T00:40:32Z

Fun automation with standard input pipes::

$ ./manage.py shell <<EOF
> import pprint
> import json
> pprint.pprint(json.dumps('fun'))
> EOF
 'fun'

https://code.djangoproject.com/ticket/27600

timgraham · 2016-12-13T20:13:10Z

Is it needed given things like https://code.djangoproject.com/ticket/25680#comment:3 ? Anyway, all features like this need a ticket and of course, documentation and tests (if feasible).

jpic · 2016-12-14T07:42:32Z

Well it's not the same, this sorts of works:

$ manage.py shell <<EOF
> print 'hi'
> 
> EOF
Python 2.7.12 (default, Nov  7 2016, 11:55:55) 
Type "copyright", "credits" or "license" for more information.

IPython 2.3.1 -- An enhanced Interactive Python.
?         -> Introduction and overview of IPython's features.
%quickref -> Quick reference.
help      -> Python's own help system.
object?   -> Details about 'object', use 'object??' for extra details.

��In [��1��]: ��hi

��In [��2��]: ��
��In [��2��]: ��
Do you really want to exit ([y]/n)?

That's better:

$ manage.py shell <<EOF
print 'hi'
EOF
hi

Thanks !

jpic · 2016-12-14T07:56:18Z

I removed the requirement to pass -c - to just mimic the behaviour of the python executable: it'll execute what it reads from stdin instead of opening an interactive shell if it finds anything was sent to stdin.

Is it ok if I wait until the ticket is accepted to write documentation and a test ?

adamchainz

Seems useful, especially since it's what python does

adamchainz · 2016-12-15T23:34:18Z

django/core/management/commands/shell.py

@@ -114,6 +116,11 @@ def handle(self, **options):
            exec(options['command'])
            return

+        if select.select([sys.stdin], [], [], 0.0)[0]:


0.0 -> 0

Also what about windows support? select module docs say at the top "Note that on Windows, it only works for sockets;" and under select func, "a straightforward interface to the Unix select() system call.". This SO post implies it's not possible to use select either.

The test passing seems to indicate that select() will ignore file handlers on windows, and basically the interpreter will never go inside that if block on windows.

So, it doesn't seem to break when being ran on windows - I'm really not a windows expert - but it's never going to work, seems like on windows, select is just going to ignore the stdin file handler and thus not return that it has data for reading.

adamchainz · 2016-12-15T23:34:28Z

django/core/management/commands/shell.py

@@ -114,6 +116,11 @@ def handle(self, **options):
            exec(options['command'])
            return

+        if select.select([sys.stdin], [], [], 0.0)[0]:
+            # Execute stdin and exit


comment before the if?

adamchainz · 2016-12-15T23:40:47Z

I imagine it could be useful in a shell script to be able to do ./manage.py shell <<< 'from models import Customer; Customer.objects.count()'

adamchainz · 2016-12-15T23:47:29Z

django/core/management/commands/shell.py

+    help = (
+        "Runs Python code passed through standard input if any otherwise "
+        "runs an interactive interpreter, like the python CLI. "
+        "It tries to use Tries to use IPython or bpython, if one of them is available."


I think the 'passed through standard input' is the most minor feature of the command, so it should come at the end of the help. Maybe take the original description and just add a sentence on the end about it instead?

adamchainz · 2016-12-15T23:49:37Z

tests/shell/tests.py

@@ -17,3 +19,13 @@ def test_command_option(self):
            )
            self.assertEqual(len(logger), 1)
            self.assertEqual(logger[0], __version__)
+
+    @mock.patch('django.core.management.commands.shell.select')


these aren't necessary, you can pass StringIOs for stdout and stdin to call_command instead. there are lots of examples in existing tests e.g. https://github.com/django/django/blob/master/tests/i18n/test_compilation.py#L41

I don't see how we could possibly pass an instance of OutputWrapper to exec(), so passing stdout to call_command has just no effect.

For stdin, I did the same hack as in createsuperuser so it's more consistent. But if you want my own opinion, I would discourage adding production code that's supposed to be run only in the test environment, I thought this was commonly known as a bad practice but am not sure anymore.

Aha I found what you should be using for both: django.utils.captured_stdin and django.utils.captured_stdout 😉

You could also open a second PR removing the hack from createsuperuser and change its tests to use captured_stdin 👍 Presumably

Ohhh right, thanks a heap Adam, it works great 🎸

jpic · 2016-12-15T23:53:24Z

Thanks @adamchainz !

Added test and documentation, but the windows test failed because of a fetch error, i'm curious to read what it would have logged if it had actually been executed.

Agreed that being able to pipe data is pretty cool, not sure if it has any use on windows installations without something like cygwin ?

jpic · 2016-12-16T10:05:33Z

Thanks @adamchainz for your feedback 🎸

Initially i have tried mocking as little as possible, and have grepped other tests for stdin/stdout to see if they where mocked and how it was done, so I tried using the stdout wrapper. The thing is that this wrapper is not used by the exec(sys.stdin.read) call so it has absolutely no effect, and needs to be mocked. Basically when trying:

    stdout = StringIO()
    stdin.read.return_value = 'print(100)'
    call_command(
        'shell',
        stdout=stdout,
    )

stdout will be empty, because exec(sys.stdin.read()) doesn't use the OutputWrapper that was passed. Same goes for stdin.

select() was mocked because select(...)[0] has to return something true-ish to enter the if block, and that can otherwise only be possible if there's really something to read in sys.stdin. I guess that's exactly why the test passes on windows, even though it would never work on windows as stated by python select's documentation note:

Note File objects on Windows are not acceptable, but sockets are. On Windows, the underlying select() function is provided by the WinSock library, and does not handle file descriptors that don’t originate from WinSock.

adamchainz · 2016-12-19T07:47:44Z

django/core/management/commands/shell.py

-    help = "Runs a Python interactive interpreter. Tries to use IPython or bpython, if one of them is available."
+    help = (
+        "Runs a Python interactive interpreter. Tries to use IPython or bpython, if one of them is available"
+        "Like the python CLI, it would read and execute code passed to standard input if any and then exit."


The 'would' sounds a bit out of place. Also the 'like the python CLI' sounds a little unnecessary now I read it again. Maybe "If there is standard input, it will be immediately executed as code and then the command will exit.".

adamchainz · 2016-12-19T07:48:28Z

tests/shell/tests.py

+
+            call_command(
+                'shell',
+            )


no need for breaking this over multiple lines now

adamchainz · 2016-12-20T10:25:19Z

django/core/management/commands/shell.py

@@ -114,6 +119,11 @@ def handle(self, **options):
            exec(options['command'])
            return

+        # Execute stdin if it has anything to read and exit
+        if select.select([self.stdin], [], [], 0)[0]:


This needs changing back to sys.stdin, it failed on Jenkins like

File "C:\Jenkins\workspace\pull-requests-windows\database\sqlite3\label\windows\python\Python27\django\core\management\commands\shell.py", line 123, in handle if select.select([self.stdin], [], [], 0)[0]: AttributeError: 'Command' object has no attribute 'stdin'

adamchainz · 2016-12-20T11:37:15Z

LGTM apart from a release note in docs/releases/1.11.txt

jpic · 2016-12-20T14:07:35Z

@adamchainz thanks a heap mate, you 🎸

timgraham · 2016-12-20T16:05:31Z

Tested on Windows using Git bash:

$ python manage.py shell << EOF
> print('foo')
> EOF
...
  File "c:\users\tim\code\django\django\core\management\commands\shell.py", line 124, in handle
    if select.select([sys.stdin], [], [], 0)[0]:
OSError: [WinError 10038] An operation was attempted on something that is not a socket

timgraham · 2016-12-20T16:06:04Z

django/core/management/commands/shell.py

@@ -7,7 +9,11 @@


 class Command(BaseCommand):
-    help = "Runs a Python interactive interpreter. Tries to use IPython or bpython, if one of them is available."
+    help = (
+        "Runs a Python interactive interpreter. Tries to use IPython or bpython, if one of them is available"


Wrap lines at 79 characters. Suggested wording:

help = ( "Runs a Python interactive interpreter. Tries to use IPython or " "bpython, if one of them is available. Any standard input is executed " "as code." )

timgraham · 2016-12-20T16:08:43Z

docs/releases/1.11.txt

@@ -314,6 +314,9 @@ Management Commands
 * The new :option:`diffsettings --default` option allows specifying a settings
  module other than Django's default settings to compare against.

+* Improved the way the :djadmin:`shell` command reads python code from standard


I'm not sure a release note is needed -- it's not really clear to me what it means. Improved? How so? If some doc update were made in ref/django-admin.txt to describe the new ability, then you could link to it from here.

@jpic add this link

timgraham · 2016-12-20T16:09:03Z

tests/shell/tests.py

+        with captured_stdin() as stdin, captured_stdout() as stdout:
+            stdin.write('print(100)\n')
+            stdin.seek(0)
+


chop blank line?

jpic · 2016-12-20T16:17:13Z

Thanks @timgraham ! it wasn't expected to work on windows because its select() implementation doesn't support file descriptors, what's your recommendation here ? Is it possible to just add linux support for now and wait for someone to contribute windows support or should I try to get this to work on windows or something else perhaps ?

timgraham · 2016-12-20T16:19:53Z

I don't have any Windows expertise to offer. If the limitation is documented and perhaps if a more helpful exception could be raised, I wouldn't block this patch on that.

adamchainz · 2016-12-20T16:21:01Z

Can't you detect windows and not even try select.select in that case? Then we'll get the old behaviour on windows which should be fine for the time being.

jpic · 2016-12-20T16:27:03Z

Thanks for your replies, I changed the patch to "if sys.platform != 'win32' and select.select..." but I'm not sure if I need to change tests too, not sure about a lot of things so I'm downloading a windows VM and try to gather more information by experimenting myself, and start some triangulation around what's best to change in docs.

adamchainz · 2016-12-20T17:16:20Z

tests/shell/tests.py

@@ -18,6 +20,7 @@ def test_command_option(self):
            self.assertEqual(len(logger), 1)
            self.assertEqual(logger[0], __version__)

+    @unittest.skipIf(sys.platform == 'win32', "Python on Windows doesn't have working select().")


not exactly true, its select() works, but not on file objects like stdin

timgraham · 2016-12-20T17:18:34Z

It's better to avoid pushing many small commits as each one starts a new build on the CI server. For a small patch like this, amending/force pushing a single commit is fine.

jpic · 2016-12-20T17:35:27Z

Thanks @timgraham, good to know ! but if i amend won't it change the sha1 and thus cause a new build anyway ? On travis we can add '[ci skip]' to the commit message to prevent triggering a build in this kind of cases.

timgraham · 2016-12-20T17:37:57Z

The workflow I use is to address all review comments locally, then push to GitHub.

jpic · 2016-12-20T17:46:29Z

Got it, thanks !

adamchainz · 2016-12-21T11:26:44Z

docs/ref/django-admin.txt

@@ -979,6 +979,20 @@ Lets you pass a command as a string to execute it as Django, like so::

    django-admin shell --command="import django; print(django.__version__)"

+.. versionadded:: 1.11


I don't think this is necessarily a 'versionadded', more of a 'versionchanged' since you could already pass code in on stdin

adamchainz · 2016-12-21T11:26:57Z

docs/ref/django-admin.txt

+    print(django.__version__)
+    EOF
+
+Note that Windows select() implementation doesn't support file descriptors, so


Since it's a function, select() should be highlighted as such, i.e.

``select()``

adamchainz · 2016-12-21T11:28:43Z

docs/ref/django-admin.txt

+.. versionadded:: 1.11
+
+Lets you pass a script to execute via standard input on Linux and have it
+output to standard output, ie::


"Lets you pass a script" is a bit fluffy. Also you mean Unix or Posix, not just Linux 😉 Perhaps something like:

"You can now pass code in on standard input and it will be executed without any extra output from the REPL. For example::"

adamchainz · 2016-12-21T11:29:38Z

docs/ref/django-admin.txt

+
+Note that Windows select() implementation doesn't support file descriptors, so
+the output on Windows hosts will contain additional, interface specific
+outputs, making output parsing a lot harder.


Maybe it should say something like

"On Windows the REPL is still outupt, due to limitations in its implementation of select()."

Output parsing is a second-order effect and probably doesn't need to be mentioned directly

jpic · 2016-12-21T16:58:38Z

Thanks again for your dedication, you're setting the bar high for other repository maintainers, keep up the great work

🎸

timgraham · 2016-12-21T22:07:11Z

merged in bf6392b, thanks!

jpic · 2016-12-22T01:00:43Z

Thanks, I hope you enjoy this too one day because you deserved it !

timgraham changed the title ~~Read from stdin for shell -c -~~ Fixed #27600 -- Allowed the shell command to read from stdin. Dec 14, 2016

adamchainz reviewed Dec 15, 2016

View reviewed changes

adamchainz changed the title ~~Fixed #27600 -- Allowed the shell command to read from stdin.~~ Fixed #27600 -- Improved the way the 'shell' command reads from stdin. Dec 15, 2016

adamchainz suggested changes Dec 15, 2016

View reviewed changes

jpic force-pushed the shell_stdin branch from 204682d to 902d2a5 Compare December 16, 2016 10:06

adamchainz suggested changes Dec 19, 2016

View reviewed changes

adamchainz suggested changes Dec 20, 2016

View reviewed changes

adamchainz changed the title ~~Fixed #27600 -- Improved the way the 'shell' command reads from stdin.~~ Fixed #27600 -- Improved the way the 'shell' command reads python code from stdin. Dec 20, 2016

adamchainz approved these changes Dec 20, 2016

View reviewed changes

Fixed #27600 -- Improved how the 'shell' command reads from stdin.

fb2dd9a

jpic force-pushed the shell_stdin branch from 5145ac9 to fb2dd9a Compare December 20, 2016 15:20

timgraham reviewed Dec 20, 2016

View reviewed changes

chopblankline

fd30f66

win32

b69a946

fixup! win32

66e9814

jpic added 2 commits December 20, 2016 18:10

tests skip win32

2128a76

Rewording for shell Command.help

c3c851b

adamchainz reviewed Dec 20, 2016

View reviewed changes

Precise skipIf win32

1d18ef4

fixup! Precise skipIf win32

8f47b1e

jpic force-pushed the shell_stdin branch from 9c64fb9 to 0e25e7a Compare December 21, 2016 11:09

adamchainz reviewed Dec 21, 2016

View reviewed changes

jpic force-pushed the shell_stdin branch 2 times, most recently from 507c61e to a848ede Compare December 21, 2016 12:19

Add docs/ref/django-admin.txt

2136c3d

jpic force-pushed the shell_stdin branch from a848ede to 2136c3d Compare December 21, 2016 12:35

timgraham closed this Dec 21, 2016

jpic deleted the shell_stdin branch December 22, 2016 01:00

adamchainz mentioned this pull request Jan 4, 2017

Fixed #27670 -- Prevented shell command from masking ImportErrors. #7776

Closed

		@@ -979,6 +979,20 @@ Lets you pass a command as a string to execute it as Django, like so::

		django-admin shell --command="import django; print(django.__version__)"

		.. versionadded:: 1.11

Fixed #27600 -- Improved the way the 'shell' command reads python code from stdin. #7681

Fixed #27600 -- Improved the way the 'shell' command reads python code from stdin. #7681

Conversation

jpic commented Dec 13, 2016 • edited

timgraham commented Dec 13, 2016 • edited

jpic commented Dec 14, 2016 • edited

jpic commented Dec 14, 2016 • edited

adamchainz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamchainz commented Dec 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpic Dec 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpic commented Dec 15, 2016

jpic commented Dec 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamchainz commented Dec 20, 2016

jpic commented Dec 20, 2016

timgraham commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpic commented Dec 20, 2016

timgraham commented Dec 20, 2016

adamchainz commented Dec 20, 2016

jpic commented Dec 20, 2016

Choose a reason for hiding this comment

timgraham commented Dec 20, 2016

jpic commented Dec 20, 2016 via email

timgraham commented Dec 20, 2016

jpic commented Dec 20, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpic commented Dec 21, 2016

timgraham commented Dec 21, 2016

jpic commented Dec 22, 2016

jpic commented Dec 13, 2016 •

edited

timgraham commented Dec 13, 2016 •

edited

jpic commented Dec 14, 2016 •

edited

jpic commented Dec 14, 2016 •

edited

jpic Dec 16, 2016 •

edited

jpic commented Dec 16, 2016 •

edited