Skip to content
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

[GSOC] feat: auto-importing django shell #18158

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

salvo-polizzi
Copy link
Contributor

@salvo-polizzi salvo-polizzi commented May 11, 2024

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

  • Auto-import all models from the current project/app.
  • Test subclassing the shell command and override a method.
  • Test importing extra-things (modules, classes, etc.).
  • Write a tutorial and document the new features.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

Copy link
Sponsor Member

@adamchainz adamchainz left a 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 !

django/core/management/commands/shell.py Outdated Show resolved Hide resolved
django/core/management/commands/shell.py Outdated Show resolved Hide resolved
django/core/management/commands/shell.py Outdated Show resolved Hide resolved
django/core/management/commands/shell.py Outdated Show resolved Hide resolved
@salvo-polizzi
Copy link
Contributor Author

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.

@adamchainz
Copy link
Sponsor Member

Yes, the tests should be within ShellCommandTestCase.

You can unit-test get_namespace() by calling it (imports = Command().get_namespace()) and making assertions on its contents. Then you can write integration tests similar to the existing ones that use command and captured_stdin to test those pathways.

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 python shell, though there’s a risk that calling the startup file, interactive hook, or readline are not a good idea during a test run.

* Deleted useless import_string function.

* Gave precedence to earlier apps.

* Loaded default imports with command option.
Copy link
Sponsor Member

@adamchainz adamchainz left a 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! 💪

django/core/management/commands/shell.py Outdated Show resolved Hide resolved
tests/shell/models.py Outdated Show resolved Hide resolved
django/core/management/commands/shell.py Outdated Show resolved Hide resolved
django/core/management/commands/shell.py Outdated Show resolved Hide resolved
django/core/management/commands/shell.py Outdated Show resolved Hide resolved
Fixed CI.

Ensured that command and stdin option had default imports.

Removed useless get_apps_and_models method.
Copy link
Sponsor Member

@adamchainz adamchainz left a 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.

django/core/management/commands/shell.py Outdated Show resolved Hide resolved
django/core/management/commands/shell.py Outdated Show resolved Hide resolved
django/core/management/commands/shell.py Show resolved Hide resolved
django/core/management/commands/shell.py Outdated Show resolved Hide resolved
tests/shell/tests.py Show resolved Hide resolved
tests/shell/tests.py Outdated Show resolved Hide resolved
tests/shell/tests.py Outdated Show resolved Hide resolved
tests/shell/tests.py Outdated Show resolved Hide resolved
@salvo-polizzi
Copy link
Contributor Author

Thanks @adamchainz again for your reviews. I left some questions in the comments

tests/shell/tests.py Outdated Show resolved Hide resolved
Copy link
Contributor

@DevilsAutumn DevilsAutumn left a 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.

@salvo-polizzi salvo-polizzi force-pushed the auto-importing-shell branch 3 times, most recently from 07c514f to 10a17aa Compare May 23, 2024 08:59
@salvo-polizzi salvo-polizzi marked this pull request as ready for review May 31, 2024 05:27
Salvo Polizzi and others added 3 commits June 6, 2024 18:28
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).
Copy link
Contributor

@DevilsAutumn DevilsAutumn left a 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.

docs/howto/overriding-shell.txt Outdated Show resolved Hide resolved
docs/howto/overriding-shell.txt Outdated Show resolved Hide resolved
docs/howto/overriding-shell.txt Outdated Show resolved Hide resolved
docs/howto/overriding-shell.txt Outdated Show resolved Hide resolved
docs/howto/overriding-shell.txt Outdated Show resolved Hide resolved
docs/howto/overriding-shell.txt Outdated Show resolved Hide resolved
docs/howto/overriding-shell.txt Outdated Show resolved Hide resolved
docs/howto/overriding-shell.txt Outdated Show resolved Hide resolved
docs/howto/overriding-shell.txt Outdated Show resolved Hide resolved
@salvo-polizzi
Copy link
Contributor Author

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.

Copy link
Sponsor Member

@adamchainz adamchainz left a 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.

Comment on lines +130 to +135
def get_and_report_namespace(self):
namespace = self.get_namespace()
self.stdout.write(
f"{len(namespace)} objects imported automatically",
self.style.SUCCESS,
)
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this method take verbosity and eliminate the repetition in the three start_ methods.

Suggested change
def get_and_report_namespace(self):
namespace = self.get_namespace()
self.stdout.write(
f"{len(namespace)} objects imported automatically",
self.style.SUCCESS,
)
def get_and_report_namespace(self, verbosity):
namespace = self.get_namespace()
if verbosity >= 1:
self.stdout.write(
f"{len(namespace)} objects imported automatically",
self.style.SUCCESS,
)

@@ -26,6 +26,7 @@ you quickly accomplish common tasks.
logging
outputting-csv
outputting-pdf
overriding-shell
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think to fit the common theme here, we should call this custom-shell.

@@ -0,0 +1,47 @@
=========================
How to override the shell
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s retitle the document “How to customize the shell command”. Whilst we do override the management command when making customizations, it’s not the clearest term.

Also, add a link from the shell section in ref/django-admin.txt to this document, with some sentence about how it’s possible to customize the shell.

How to override the shell
=========================

Django provides functionality to auto-import project models in the shell. In your project, you might want to either avoid importing models or include additional imports in the shell. You can do this by subclassing the :djadmin:`shell` command and overriding a method.
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap lines at 80 characters: https://docs.djangoproject.com/en/dev/internals/contributing/writing-documentation/#guidelines-for-restructuredtext-files

Most text editors allow configuring a line to appear in that column, so you can visually wrap it. You can also find automatic wrapping commands or extensions for most editors.


Django provides functionality to auto-import project models in the shell. In your project, you might want to either avoid importing models or include additional imports in the shell. You can do this by subclassing the :djadmin:`shell` command and overriding a method.

Setting up the app directory
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can drop this section and just have a one-sentence link to howto/custom-management-commands. Developers doing shell customization presumably have some level of experience and don’t need everything spelled out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants