Make the system check look at the types of settings?

Django Internals
22

I’m talking about situations where forcing lazy settings evaluation at the initial stage (where checks are performed) can can issues. For example, we have a setting that depends on something else that is available when the setting is used for the first time, but not at the initial stage.

23

CACHE_MIDDLEWARE_SECONDS is just used as one example for the unittest it works with any other setting as well. So for example there is some existing test ReverseLazySettingsTest, that creates a settings.py on the fly and contains LOGIN_URL=reverse_lazy('login') as a setting, which also will not be flagged by the check.

I’m not proposing to change the current settings system at all and I still try to understand the “likely pushbacks” to be expected, given that lazy evaluation is now not flagged by the additional checks I’m voting for.

24

I’m not proposing any change to the settings system at all and only voting for additional checks to discover misconfigured settings.

25

I understand that and as I said a few times now, have addressed this, so that any lazy evaluation (using django.utils.functional) used in settings will not be flagged and just ignored.

I was talking about any problems you have seen besides that.

26

Yes, I was just clarifying that I agreed with Natalia’s point there. :+1:

27

Maybe I just have to reiterate the following:

  • I’m voting for adding additional checks for the settings, so that misconfiguration is less likely
  • I’m trying to do that by comparing the settings with the default settings (django.conf.global_settings). First by value and then by type (using isinstance). If the setting uses some lazy evaluation, then the setting will be ignored
  • These are only additional checks. I don’t change the settings system at all.
  • These additional checks produce only Warnings and don’t prevent Django from starting at all
  • These additional checks can be easily silenced
  • These additional checks have little impact on startup time

In addition:

  • True, there might be better and more radical approaches to the problem. But it seems they would require much more work and are targeted at much more, than my PR. I think my PR would be a way to a “low hanging fruit” instead of planting a new tree.
  • I think the case with lazy evaluation is covered now as well, as long as you won’t try to use another Promis/Proxy/Lazy class of your own, where I think it’s fair to raise a warning during system checks.
  • The PR could still have issues, but I was asking for examples other than lazy evaluation.

I still haven’t seen any real reason, why this would be a bad idea and the PR being shot down just because of “I don’t like it, because there could be unnamed problems” is a bit frustrating.

28

I also agree that the settings while not perfect are good. If we’re searching for an improvement through type hinting, what about expecting the programmer to add the type?

CACHE_MIDDLEWARE_SECONDS: int = 300

I usually find code like this noisy (type hinters can figure out that it is an int, so the type hint is redundant). However, from a pragmatic perspective, the type hint is actually acting as documentation. In production, we might really grow to have code that looks like this (to convert string env vars to ints).

CACHE_MIDDLEWARE_SECONDS: int = getint("CACHE_MIDDLEWARE_SECONDS", 500)

I understand the user is still capable of writing the wrong type hint that’s not compatible with the framework. Also, I’m not even sure what role the framework itself plays with this suggestion, but I wanted to share this because it seemed overlooked.

29

Sorry, I just realized, that your comment was not about the possible overlap, but that there are other type checks in place for the admin.

Django is restricting the types of instance variables on subclasses of admin.ModelAdmin. For example if you set fields to something else than a list or tuple, Django will issue an ERROR and will fail to start.

So even if the following is working fine, Django won’t let you do this:

fields_lazy = lazy(lambda x: ["name", "title"][:x], list)


@admin.register(LazyModel)
class LazyAdmin(admin.ModelAdmin):
    fields = fields_lazy(1)

You could argue, that it’s very unlikely that anybody would need that and the common case is to use a list or tuple for fields.

But I think the same can be said for the settings. In the common case the types of settings are matching the types of the default values for these settings. And with the PR the rare case, that lazy evaluation is used, would still be possible without an error or warning raised. In contrast to the checks for the admin, where you would need to --skip-checks entirely (or silence that particular check) to make Django start with the above fields definition.