-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed #35515 -- Added auto-importing to shell command. #18158
base: main
Are you sure you want to change the base?
Conversation
4f90a52
to
4d9eb51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up from my post on the forum :) You got this !
4d9eb51
to
abc0225
Compare
Thank you so much, @adamchainz, for the feedback. Currently, I'm concentrating on writing tests for these features. Do you have any references or suggestions on where to start? I'm considering creating new functions within the shellCommandTestCase to test the auto-imports. |
Yes, the tests should be within You can unit-test It seems the existing tests don't actually cover launching the various shells. I think it’s worth trying adding coverage, at least for the default |
* Deleted useless import_string function. * Gave precedence to earlier apps. * Loaded default imports with command option.
6ebc3af
to
b0a2562
Compare
Tried to fix CI error.
b0a2562
to
aa2080a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work getting some tests written! 💪
ed82b68
to
cc7c5e8
Compare
Fixed CI. Ensured that command and stdin option had default imports. Removed useless get_apps_and_models method.
ca191f6
to
c2a6351
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your continued iteration. I hope you learn some things from my comments here.
Thanks @adamchainz again for your reviews. I left some questions in the comments |
df3323c
to
a1582f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @salvo-polizzi , Thankyou for your constants efforts on this! And special thanks to Adam for reviewing. This PR looks in a great shape already. 🥇
I finally have some time to push this further.
07c514f
to
10a17aa
Compare
Deleted useless update_globals method. Deleted useless test. Minor improvements. Setted maxDiff to None. Updated installed apps for tests. Tested apps precedence using reversed. Fixed CI. Fixed CI (II). Fixed CI (III).
bcf6559
to
4de4e1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @salvo-polizzi , I left some comments on the docs. Also, it will be better to add one more docs section under how to override the shell
to show how to override the shell to use it for a custom python shell runner
.
Hi @DevilsAutumn, thanks for your review. I've started writing the new section on how to override the shell for a customized shell runner. I'll push it in the next few days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! I just left DjangoCon Europe this morning. I have been discussing this project with other attendees and I only heard positive reactions.
It’s great to see further progress on this PR.
I’ve added some specific comments, but I think now is a good time to also address a couple of pieces of groundwork.
First, we don’t have a Trac ticket, and we should do for any notable change. Salvo, can you create one on Trac, mention it in the PR description (update to match the PR template), and retitle the PR/commit something like “Fixed # -- Added auto-importing to shell command.”.
The ticket description doesn’t need to be long and can link to the wiki and your proposal.
Second, let’s add some missing test coverage in a separate PR. Specifically, I noted that we don’t have any test coverage of the methods that run shells: ipython()
, bpython()
, and python()
.
Let’s add those tests in a separate PR with a title like “Refs # -- Improved test coverage of shell command.”. The tests can use this technique to mock the imports of the IPython
and bpython
, so they don’t actually launch the shell, and don’t need the corresponding packages installed. Similar mocking can be done for the code
module’s code.interact()
in python()
.
The tests in this PR can then build on that, checking that the right arguments are passed. They won’t be perfect, since we won’t actually be testing the integration, but they’ll be better than nothing.
Hi @adamchainz, I've just changed the PR title and created a Trac ticket for this new feature. It's great to hear that this functionality is very well appreciated by other Django users and contributors, and I'm glad to contribute to this project. In the next few days, I'll work on writing tests to improve test coverage for methods that run the shell. I'm looking forward to seeing how this will turn out. |
8bc41e5
to
5079fea
Compare
Wrapped columns to 80 lines. Fixed underline Reformatted docs with black Added a reference to new docs in django-admin.
5079fea
to
36142cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant, some more iterations here.
start_ipython(argv=[]) | ||
start_ipython( | ||
argv=[], | ||
user_ns=(self.get_and_report_namespace(options["verbosity"])), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop unnecessary parentheses
user_ns=(self.get_and_report_namespace(options["verbosity"])), | |
user_ns=self.get_and_report_namespace(options["verbosity"]), |
@@ -0,0 +1,42 @@ | |||
============================================= | |||
How to customize the :djadmin:`shell` command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does a link in a title work well? I would guess not. Did you build the docs locally?
Regardless, let’s use plain text in the title:
How to customize the :djadmin:`shell` command | |
How to customize the ``shell`` command |
The link can live only in the body text below.
|
||
This shell also auto-imports models from your project for you. Additionally, you | ||
can customize which imports you want or add a new shell runner by following the | ||
:doc:`custom-shell</howto/custom-shell>` guide. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we would want a versionchanged
annotation to highlight the new auto-import feature - search other docs for examples
@@ -94,3 +122,77 @@ def test_shell_with_bpython_not_installed(self, select): | |||
# returns EOF and so select always shows that sys.stdin is ready to read. | |||
# This causes problems because of the call to select.select() toward the | |||
# end of shell's handle() method. | |||
|
|||
def test_get_namespace(self): | |||
with self.settings( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the @override_settings
decorator on the test instead, to avoid a level of indentation
) | ||
|
||
def test_get_namespace_precedence(self): | ||
class Marker(models.Model): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the @isolate_apps
decorator when creating temporary models, to avoid the model being “leaked”
cmd = shell.Command() | ||
namespace = cmd.get_namespace() | ||
|
||
self.assertTrue(Marker in namespace.values()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use specific assertion functions so the message is better on failure (for all below assertions)
self.assertTrue(Marker in namespace.values()) | |
self.assertIn(Marker, namespace.values()) |
self.assertFalse(Marker in namespace.values()) | ||
|
||
def test_overridden_get_namespace(self): | ||
with self.settings( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again use @override_settings
"django.contrib.contenttypes", | ||
] | ||
): | ||
cmd = overridden_shell.Command() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this class definition within the test function, so there’s no extra file. The class doesn’t need to live within a management command module because it’s never called directly.
@@ -111,10 +115,33 @@ def python(self, options): | |||
# Start the interactive interpreter. | |||
code.interact(local=imported_objects) | |||
|
|||
def get_and_report_namespace(self, verbosity): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is missing return namespace
at the end. When you update the tests added in #18265, make sure that they would fail if we didn't catch this :)
Also please make sure to test your changes manually with Python, IPython, and bpython, to ensure they work. Using mocks in tests means there is always a chance of uncaught issues.
def handle(self, **options): | ||
# Execute the command and exit. | ||
if options["command"]: | ||
exec(options["command"], globals()) | ||
exec(options["command"], {**globals(), **self.get_namespace()}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, thinking about it, I am not sure it makes sense to add globals()
here, or below for stdin.
I inspected the contents by adding these lines at the top of handle()
:
from pprint import pp
pp(globals())
The result:
$ ./manage.py shell
{'__name__': 'django.core.management.commands.shell',
'__doc__': None,
'__package__': 'django.core.management.commands',
'__loader__': <_frozen_importlib_external.SourceFileLoader object at 0x101732c00>,
'__spec__': ModuleSpec(name='django.core.management.commands.shell', loader=<_frozen_importlib_external.SourceFileLoader object at 0x101732c00>, origin='/Users/chainz/Documents/Projects/django/django/core/management/commands/shell.py'),
'__file__': '/Users/chainz/Documents/Projects/django/django/core/management/commands/shell.py',
'__cached__': '/Users/chainz/Documents/Projects/django/django/core/management/commands/__pycache__/shell.cpython-312.pyc',
'__builtins__': {...},
'os': <module 'os' (frozen)>,
'select': <module 'select' (built-in)>,
'sys': <module 'sys' (built-in)>,
'traceback': <module 'traceback' from '/Users/chainz/.local/share/mise/installs/python/3.12.2/lib/python3.12/traceback.py'>,
'BaseCommand': <class 'django.core.management.base.BaseCommand'>,
'CommandError': <class 'django.core.management.base.CommandError'>,
'OrderedSet': <class 'django.utils.datastructures.OrderedSet'>,
'Command': <class 'django.core.management.commands.shell.Command'>}
I elided the __builtins__
key, because it’s big, and exec
adds it by default, if missing.
The remaining __
/“dunder” names are related to the shell
module, and so shouldn't really be used by passed-in code.
The non-dunder names (os
, sys
, etc.) are simply whatever shell
happens to import. Naturally, they’ve changed over Django’s history.
If we drop the **globals()
from these two places, all those names will go away, potentially breaking scripts using -c
or stdin without explicit imports. But the namespace will be a lot more logical, and now would be the best time to change it. Users can always fix things by following the guide to override get_namespace()
and add back the imports they rely on...
What do you think? Asking @DevilsAutumn too.
Branch description
This would be an update of the existing Django shell that auto-imports models for you from your app/project. Also, the goal would be to allow the user to subclass this shell to customize its behavior and import extra things.
TODO
Checklist
main
branch.