Organizing the built-in system check framework's hint messages

[Quentin Fulsher]

Hi all, 

Recently I was trying to fix a bug that required me to alter the hint message of the `fields.E306`[1] system check error[2]. The bug in question added a condition to a check which if failed caused the instantiation of a `django.core.checks.Error` object. In order to change the `Error`'s hint properly I would have to change the hint in 3 different places. Once where the `Error` was instantiated, in the documentation, and in it's tester. This was a rather tedious task and if done improperly can lead to differing error messages. Rather than just passing the hint in as an argument to the constructor I think it would be better if the Error object had the option to lookup it's error messages rather than just having them given. 

It wouldn't take much effort to create a dictionary of `id: message` pairs which `Error` objects could use to lookup their messages/hints. In the event that an `Error` is instantiated with no hint/message given it could perform a quick check to see if it is logged in the dictionary. This would not add any significant overhead to the process and eliminate some possibility of human error. 

This situation becomes even more tedious if you have more than one place where the error is instantiated or tested. This could also help with documentation because all of the builtin error messages would be stored in a central location.

In any case, let me know what you guys think. I know its kind of a non-issue as the current system works fine. I just thought there was some room for improvement. Thanks!

[1] https://docs.djangoproject.com/en/1.9/ref/checks/#fields

[2] https://docs.djangoproject.com/en/1.9/topics/checks/

[Tim Graham]

The security checks errors are defined as module level constants to avoid some redundancy since these can be imported in tests: https://github.com/django/django/blob/0eac5535f7afd2295b1db978dffb97d79030807e/django/core/checks/security/base.py.

If you feel your approach would be an improvement, maybe you could put together a quick proof of concept so it's a bit easier to understand?

[Quentin Fulsher]

Here is a super quick proof of concept that I put together. I just branched my fork of django and added a little to it. Here is the comparing changes page[1].

Quick summary of changes: I created a dictionary that would contain the (id: message) pairs. I also modified the CheckMessage.__init__ method so that it will attempt to find a hint message for the id that was passed to it. If no key was found then it continues as normal. This allows me to comment out the hint parameter when it is created but still be able to pass when it is run through its normal tests.

[1] https://github.com/django/django/compare/master...inondle:hints

[Michal Petrucha]

On Thu, May 19, 2016 at 02:56:48PM -0700, Quentin Fulsher wrote:
> Here is a super quick proof of concept that I put together. I just branched
> my fork of django and added a little to it. Here is the comparing changes
> page[1].
>
> Quick summary of changes: I created a dictionary that would contain the
> (id: message) pairs. I also modified the CheckMessage.__init__ method so
> that it will attempt to find a hint message for the id that was passed to
> it. If no key was found then it continues as normal. This allows me to
> comment out the hint parameter when it is created but still be able to pass
> when it is run through its normal tests.

Personally, I'm not entirely convinced – this approach might work OK
for hints, which are just static strings, but not so much for the
error messages themselves, which (at least in the case of field
checks) contain arbitrary string interpolation. I can see three
options here:

1. Put both hints, and error message format strings into a central
place. This way, each is only defined in one place, which is nice,
but with the arbitrary string interpolation, we'd need to extend
the CheckMessage API to handle it after retrieving the message
template. Also, this way, the format strings and their arguments
will reside in different files, which doesn't seem very nice.

2. Use a central definition for hints, but keep messages as they are.
No special handling of string interpolation would be necessary, but
we lose the benefits of having a single definition for error
messages.

3. Keep things the way they are.

Personally, I don't like option 2 much, because it solves only half of
the problem, but I'm not certain option 1 is worth it either. Random
things to note:

- As Tim pointed out, certain parts of the check framework already do
something similar, but in a different way that doesn't seem to mix
well with string interpolation.

- Different parts of the check framework use different styles of
string formatting, which means if we try to tidy up the message
format strings, we'd have to settle on a single style.

I do agree that having to change the message in three places is a bit
annoying, though.

I hope this makes at least some sense...

Cheers,

Michal