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

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

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.

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.

Thank you @salvo-polizzi for this work, I'm performing an in-depth review with the goal to land in the next 1-2 weeks. Sarah and I would love to have this feature merged before the 5.2 feature freeze. I'm also doing some trivial fixes and pushes, such as resolving conflicts.

There is one issue from the printed models (with -v 2) that REALLY bothers me which is that the shown imports does not follow the usual import isort guidelines that we use. (cc/ @adamchainz)

(I'm adding this comment in isolation to start this conversation, other comments may come later as comments or new revnos pushed.)

Specifically, for a simple project with a single custom Django app (testapp), this is what I see:

(djangodev-3.13) nessita@picasso:~/fellowship/projectfromrepo$ python -Wall manage.py shell -v 2
12 objects imported automatically
from testapp.models import BasicModel
from django.contrib.sessions.models import Session
from django.contrib.contenttypes.models import ContentType
from django.contrib.auth.models import User, Group, Permission
from django.contrib.admin.models import LogEntry
import django.contrib.admin.models as admin_models
import django.contrib.auth.models as auth_models
import django.contrib.contenttypes.models as contenttypes_models
import django.contrib.sessions.models as sessions_models
import testapp.models as testapp_models
Python 3.13.0 (main, Oct  8 2024, 08:51:27) [GCC 13.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> 

(MY EYES MY EYES) I would rather we indent one level in for the list, and we sort with isort if possible?

(djangodev-3.13) nessita@picasso:~/fellowship/projectfromrepo$ python -Wall manage.py shell -v 2

Automatic imports: 12 objects imported automatically.

    import django.contrib.admin.models as admin_models
    import django.contrib.auth.models as auth_models
    import django.contrib.contenttypes.models as contenttypes_models
    import django.contrib.sessions.models as sessions_models
    from django.contrib.sessions.models import Session
    from django.contrib.contenttypes.models import ContentType
    from django.contrib.auth.models import User, Group, Permission
    from django.contrib.admin.models import LogEntry

    import testapp.models as testapp_models
    from testapp.models import BasicModel

Python 3.13.0 (main, Oct  8 2024, 08:51:27) [GCC 13.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> 

Also, I think I have all the comments in the PR history and I did see the mention of including extra imports, but I think I don't see those in this PR? Are the ones that we agreed on https://forum.djangoproject.com/t/default-automatic-imports-in-the-shell/33708 being added in a follow up PR?

Thanks everyone!!!

Hi @nessita,
Thank you for your valuable review. I apologize for not providing feedback over the past few weeks; I've been quite busy.

Regarding the sorting of imports, I’d like to hear Adam’s thoughts, but personally, I think using isort could be a good solution.

The PR related to default imports is #18560. I haven’t worked on it yet because I was waiting for this PR to be merged first, so we can build the new changes upon it.

Hi @nessita, Thank you for your valuable review. I apologize for not providing feedback over the past few weeks; I've been quite busy.

No worries!

Regarding the sorting of imports, I’d like to hear Adam’s thoughts, but personally, I think using isort could be a good solution.

So I've been thinking about this, and I think that we should only sort if the env already has isort available, because at this stage we depend on the env created for the project and not in Django's. Does this make sense?

I also think we should indent the listing, I can push a proposal soon if you don't object.

The PR related to default imports is #18560. I haven’t worked on it yet because I was waiting for this PR to be merged first, so we can build the new changes upon it.

Thank you! Makes sense to not update that until this one is completed and merged.

Note that the current order printed is not the same as the order imported and we should probably update this (as duplicate model names overwrite eachother)

Note that the current order printed is not the same as the order imported and we should probably update this (as duplicate model names overwrite eachother)

I was thinking whether we could only show the final list of "really" imported paths, that would mean removing from the list the shadowed imports.

@salvo-polizzi Before changing anything, do you think that is doable and reasonably simple? Or does it add too much complexity?

I was thinking whether we could only show the final list of "really" imported paths, that would mean removing from the list the shadowed imports.

Do you mean to only show the models that aren't overwritten?

So I've been thinking about this, and I think that we should only sort if the env already has isort available, because at this stage we depend on the env created for the project and not in Django's. Does this make sense?

I'm sorry if this is straightforward but how can we check if the env has isort available and then use it?

I'm sorry if this is straightforward but how can we check if the env has isort available and then use it?

You could probably take some inspiration from run_formatters(), and possibly #18597.

You could probably take some inspiration from run_formatters(), and possibly #18597.

Thank you for this valuable advice! I do have some concerns about how we should run isort in the shell prompt. Am I correct in understanding that isort can only be run on Python files?

One potential solution could be to import isort and use the isort.code() function. However, I don't think this approach is viable, as it would shift away from relying on the user's environment and instead introduce additional complexity to the Django environment.

Oh, good point that we are unlikely to want to launch an executable like we're doing with black. Sorry, I hadn't thought that far ahead. I bet just try / except'ing import isort would do?

Could I ask you to flesh out a bit what you mean when you say the difference between django & user environments? Are you thinking about having to adjust the development dependencies for django itself to run the tests for this feature?

Could I ask you to flesh out a bit what you mean when you say the difference between django & user environments? Are you thinking about having to adjust the development dependencies for django itself to run the tests for this feature?

Yes, that was what i meant - I didn't expressed myself clearly.

@nessita should I proceed with sorting the imports, or should we wait for more feedback?

@nessita should I proceed with sorting the imports, or should we wait for more feedback?

I'm very much in favor of this, could you please confirm if this is doable in a reasonable timeframe? Also, do you have the availability? And lastly, do you think this would be a desirable improvement?

Thanks!

I'm very much in favor of this, could you please confirm if this is doable in a reasonable timeframe? Also, do you have the availability? And lastly, do you think this would be a desirable improvement?

Thanks!

Yes, I think it is a good improvement showing the imports in the right order and I think it is a pretty straightforward change to implement (i'm just about to complete the implementation 😊) . Let me know if you have any additional thoughts or requirements!

I'm very much in favor of this, could you please confirm if this is doable in a reasonable timeframe? Also, do you have the availability? And lastly, do you think this would be a desirable improvement?
Thanks!

Yes, I think it is a good improvement showing the imports in the right order and I think it is a pretty straightforward change to implement (i'm just about to complete the implementation 😊) . Let me know if you have any additional thoughts or requirements!

Thank you @salvo-polizzi! I ran out of time this week but this PR will be my priority next week. I may push some fixes and let you know. Until then, would you please consider fixing the conflict in the release notes?

@nessita Following our discussion on the forum, I plan to complete all the requested changes by the end of this week.

@nessita Following our discussion on the forum, I plan to complete all the requested changes by the end of this week.

Thank you so much, I really appreciate this!

Thank you, @nessita, for your work on this PR—I really appreciate it.

I understand that the other two are the customized MyClass and constant, but somehow these are not showing in the "full list" and it feels unexpected. Have you given this any consideration?

Regarding MyClass and constant not appearing in the "full list," I believe this is due to our decision not to show nested classes or variables in the list. Here's the link to the discussion for reference. Let me know if I misinterpreted your question.

Regarding MyClass and constant not appearing in the "full list," I believe this is due to our decision not to show nested classes or variables in the list. Here's the link to the discussion for reference. Let me know if I misinterpreted your question.

@salvo-polizzi Thank you, this answers my question. I guess then that my proposal of appending "Full list" should be revisited since the list that we show is not a complete list. Perhaps we should say "Including" or similar, like Sarah suggested? Do you have a preference/recommendation?

Perhaps we should say "Including" or similar, like Sarah suggested? Do you have a preference/recommendation?

I think that something like "including" should be good.

Thanks @pauloxnet for your valuable feedback 👍