Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#22422 closed Cleanup/optimization (fixed)

RegistryNotReadyException in django.apps.registry.Apps.check_ready()

Reported by: valberg Owned by: nobody
Component: Documentation Version: 1.7-beta-1
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


Instead of using RuntimeError in django.apps.registry.Apps.check_ready(), we should use a django defined exception (possible inheriting from RuntimeError). This way, we can easily catch the exception at the level of occurence, and reraise it with an informative message.

For instance, using django.utils.translation.ugettext in raises a RuntimeError. This does not aid in explaining where the error occurs. Here we could raise RegistryNotReadyException('Use django.utils.translation.ugettext_lazy instead of ugettext').

In the spirit of the new checks framework, let us make it easier for developers to recognize errors and try to inform them how they might be able to fix them. For instance importing ugettext instead of ugettext_lazy in raises an unexplained RuntimeError when running the 'check' command.

Change History (9)

comment:1 Changed 4 years ago by Aymeric Augustin

I'm not sure I understand your proposal. Do you suggest to wrap every call to the app registry with a try / except and reraise errors with a specific error message?

First, while Django has had a very bad habit to try to be helpful with exceptions by catching them and reraising a different exception, this hasn't delivered the expected results. In practice it tends to hide the real cause make it more complicated to understand errors. Getting rid of this pattern is even a GSoC project topic! We have tracebacks to figure out where errors come from.

(Once we drop support for Python 2, this argument won't apply anymore. But that isn't for today.)

Second, there's over 150 such occurrences in the code base, and I find it unrealistic to hope that every future maintainer will properly wrap every call to the app cache like you suggest.

Using a custom exception for exceptions thrown by the app registry may still make it slightly easier to search for related information in the documentation or on the Internet.

comment:2 Changed 4 years ago by valberg

It might well be that's just me that is not that good at deciphering tracebacks :)

The idea was to make it easier to figure out what is actually going wrong, as you write in the end of your comment.

I can definitely see the point that it would add more complexity for future maintenance.

Maybe it's more down to a documentation issue. I solved my problem reading

Including something about these RuntimeErrors in would definitely be an acceptable solution seen from my point of view :)

Last edited 4 years ago by valberg (previous) (diff)

comment:3 Changed 4 years ago by Aymeric Augustin

Component: Core (Other)Documentation
Triage Stage: UnreviewedAccepted

I looked at this again today and I don't think it's very useful to change raise RuntimeError("App registry isn't ready yet.") to raise AppRegistryNotReady("App registry isn't ready yet."). Putting the error message into Google takes you straight to the 1.7 release notes, which are the best resource on this problem at this time.

The application loading process isn't documented. It would be worth saying at least a few words in ref/applications.txt and copying the troubleshooting advice there.

comment:4 Changed 4 years ago by valberg

I've just made a pull request ( Basically just copying from the release notes to the reference page. I'm not knowledgable enough about the loading process to write it down.

I know it's a bit redundant to have this in both the release notes and the reference page. But it just seems right to be able to rely on the documentation without the release notes (even though they technically are a part of the documentation).

comment:5 Changed 4 years ago by Tim Graham

Has patch: set

I think I would prefer linking the release notes to the text in the documentation so as we make additions and/or corrections we don't need to update two places. The PR should be against master, then we backport from there.

comment:6 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In deb561bbe2dbacac32d65272beab3a7708e2c31d:

Fixed #22422 -- Moved information about the application loading process to refs/applications.txt.

comment:7 Changed 4 years ago by Tim Graham <timograham@…>

In bde1bc66724db34dcde598f064448da1f2a8a810:

[1.7.x] Fixed #22422 -- Moved information about the application loading process to refs/applications.txt.

Backport of deb561bbe2 from master

comment:8 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

In 0315f01087d75f68654f62883a9f36a0f9f71676:

Fixed a confusing heading in applications docs.

Refs #22422.

comment:9 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

In 788fce4c917eee27b10f38f7eb0b75cd2af3ec43:

[1.7.x] Fixed a confusing heading in applications docs.

Refs #22422.

Backport of 0315f01 from master

Note: See TracTickets for help on using tickets.
Back to Top