*Progress.*

- Deprecated `requires_model_validation` flag and `validate` method (both
  `BaseCommand` members) in favour of new `requires_system_checks` flag and
  `check` method.

- Finished working at `contenttypes` tests.

- Improved code thanks to Preston Holmes comments. Deleted dead code and 
added
  some new tests to improve code coverage.

It'd be nice to have checks of clashes between GenericForeignKey and
accessors. I didn't implemented it because little time left and I need to
hurry up.

When it was easy, I've added new tests to improve code coverage. However, 
there
are still some tests that would be nice to have:

- Test for raising an error while using an unique varchars longer than 255
  characters under MySQL backend. [1]

- Test for `ImageField`. When `Pillow` is not installed and `ImageField` is 
in
  use, an error should be raised. This should be tested. [2]

- Test for raising warning/ImproperlyConfigured in `BaseCommand.__init__`. 
[3]

[1] 
https://github.com/chrismedrela/django/blob/149800a8136ce839903f0fe9af7f548973da9244/django/db/backends/mysql/validation.py#L6
[2] 
https://github.com/chrismedrela/django/blob/149800a8136ce839903f0fe9af7f548973da9244/django/db/models/fields/files.py#L447
[3] 
https://github.com/chrismedrela/django/blob/149800a8136ce839903f0fe9af7f548973da9244/django/core/management/base.py#L202

*Filtering and silencing errors/warnings.* We need two features: ability
to run only a subset of checks (aka filtering errors) and ability to silence
some errors.

Silencing is easy. Every warning will have a unique name like "W027". The 
format
of the name is letter "W" followed by a unique number. The system check
framework is open ended and third-party apps can register its own checks. 
For
warnings raised by these apps, "Wnumber.applabel" (e.g. "W001.myapp") style 
will
be used. Of course, everything can be changed and I'm open to yours 
suggestions,
so feel free to comment and criticize it.

There will be a new setting called `SILENCED_WARNINGS`. If you want to 
silence a
warning, you put its name in this setting, e.g.:

    SILENCED_ERRORS = [
        'W027',
    ]

Only light messages (warnings, infos and debugs) can be silenced.

Running only a subset of check is a more complex task. We can associate 
every
check with a set of tags (that can be done while registering checks). To run
only checks tagged "mytag" or "mysecondtag", you need to type:

    manage.py check mytag mysecondtag

However, we would like to run checks of an app (or a set of apps):

    manage.py check auth admin

This is hard, because checks are no longer app-specific. I propose to solve 
this
by passing an `apps` optional keyword argument to every check function. The
function should only validate specified apps (or all apps if apps==None). 
The
only problem is how to determine if we deal with a tag or app name? Consider
this command:

    manage.py check auth mytag

This should run all checks tagged "mytag". Only messages for `auth` app 
should
be reported. I propose to collect all tags while registering check 
functions and
if a string is one of them, then interpret it as a tag, otherwise assume 
it's an
app name (and check if an app with given name exists).

*Problems/questions.*

1. We decided to mimic message and logging framework, so every error is 
tagged
with a level. Its value is an integer indicating how important and serious 
is
a message. There are five predefined values: CRITICAL, ERROR, WARNING, INFO,
DEBUG. However, Preston Holmes noticed that this is unpractical because we
need only errors and warnings. I think we should discuss again if it's worth
mimicing these frameworks.

2. On the pull request we started discussing names. "System check" is 
better than
"check" but it suggest that it's connected with hardware. Preston proposed
"Startup checks". I don't have strong opinion. To be honest, I don't thing
"startup checks" is much more better than "system checks" so I will leave 
it as
it is, but I'm still open to suggestions and I would like to see yours 
opinion.

3. There are some problems with moving custom User model checks. Their 
first source
was that some tests override `INSTALLED_APPS` setting but don't override 
list of
registered checks. So checks of `auth` app were registered (because the app 
was
imported by some other tests), but this app wasn't installed. This ended in 
an
error, because checks of `auth` try to load User model which is, by default,
`auth.User` and `auth` is not installed. I've solved this by overriding 
list of
registered checks (I've introduced `override_system_checks` decorator).

However, there is still one red test -- `admin_scripts.test_complex_app` 
[4].
The problem is that this test spawns a new Django process and the decorator
cannot affect this new process. This test installs two apps (`complex_app` 
and
`simple_app`) and doesn't install `auth` app. However, `auth` app is 
imported,
because `complex_app` imports `admin` app which imports `auth` app. The
simplest solution is just to add `auth` app to `INSTALLED_APPS`.

[4] 
https://github.com/chrismedrela/django/blob/149800a8136ce839903f0fe9af7f548973da9244/tests/admin_scripts/tests.py#L1078

This shows a drawback of the current registration approach. Every time an 
app
is imported but not installed, its checks are registered, but they shouln't
be. I think we should rethink how checks should be registered. An 
alternative
can be Russell's solution -- create a new setting `SYSTEM_CHECKS` containing
list of all checks. This has one small drawback -- when you want to install 
an
app with custom checks, you need to put it in both `INSTALLED_APPS` and
`SYSTEM_CHECKS`; but we can overcome it by implicit extending 
`SYSTEM_CHECKS`
with app-specific checks. So, I can write:

    INSTALLED_APPS = [
        'auth',
    ]

And all checks of `auth` app will be automagically registered. Of course, we
need some public API for retrieving app checks. We can reuse the existing
registering mechanism and add `app` argument to `checks.register` function
indicating that this one particular check should be run only if app `app` is
installed.

4. In my last post I mentioned that there is a problem with
`BaseCommand.verbosity`. I tried to convert its default value ('1') into an
unicode, but I failed. This test command [5] prints its arguments to stdout,
so the output is something like this:

    ... options=[('pythonpath', None), ... ('verbosity', '1')]

But when you change `verbosity` from a bytestring to an unicode then the 
output
is (only under Python 2):

    ... options=[('pythonpath', None), ... ('verbosity', u'1')]

Russell proposed to replace `sorted(options.items())` in [5] with:

    sorted((text_type(k), text_type(v)) for k, v in options.items())

This is a solution, but it makes tests less strict. There is no way to
distinguish between `None` value and "None" string -- both are printed in 
the
same way.

I spent some time to find a better approach but with no success. The least
insane way is to check if the object to print is an unicode and if it is,
print it in the Python 3 style (without starting "u"); otherwise print it
normally.

Note that this is a more general problem. I've hit the same problem when I 
tried
to write a test for `CheckMessage.__repr__` method. I wrote:

    def test_repr(self):
        e = Error("Error", hint="Hint", obj=None)
        expected = "<CheckMessage: level=40, msg='Error', hint='Hint', 
obj=None>"
        self.assertEqual(repr(e), expected)

But under Python 2, `expected` variable should be:

        expected = "<CheckMessage: level=40, msg=u'Error', hint=u'Hint', 
obj=None>"

[5] 
https://github.com/chrismedrela/django/blob/149800a8136ce839903f0fe9af7f548973da9244/tests/admin_scripts/management/commands/base_command.py#L17

5. I've deleted one filter in [6]. I think it's useless. If
`self.related.get_accessor_name` is None, then `self.rel.is_hidden()` is 
true,
so this is a dead if. Am I right?

[6] 
https://github.com/chrismedrela/django/commit/be54f9411526c660e93ae67cbcddbd173a02b81a

6. When `ForeignKey.foreign_related_fields` is a list of more than one 
element? If
it's always a one-element list, then we don't need lines 1443-1457 in
`ForeignKey._check_unique_target`. [7]

[7] 
https://github.com/chrismedrela/django/blob/149800a8136ce839903f0fe9af7f548973da9244/django/db/models/fields/related.py#L1443

7. It looks like the code I've deleted in this commit [8] was dead, because 
if you
refer to an inherited field, an FieldDoesNotExist exception is raised by
`get_field` method. Is that right?

[8] 
https://github.com/chrismedrela/django/commit/e41ae69a75d85c0325a61fdcf00cd03ce1692913

8. I've added a new check. If you're using a `GenericRelation` but there is 
no
`GenericForeignKey` on the target model, an warning is raised. This check 
was
implemented in this commit [9]. It uses `vars` builtin function to check if 
the
target model has a `GenericForeignKey`. This is ugly, but I don't see a 
better
approach.

[9] 
https://github.com/chrismedrela/django/commit/ab65ff2b6d6346407a11a72c072e358c7b518cf9#L1R397

*Schedule.* There is little time to the end of GSoC -- only 5 weeks and I 
will
work 50% of full speed after September 6, because I'm going on holiday.

- Rewriting admin checks (1 week).
- Implementing filtering and silencing errors (1 week).
- Merging django-secure (2 weeks?!).
- Polishing doc, last review (1 week).

I will implement filtering and silencing errors before merging django-secure
because these are a must if we want to merge django-secure.

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.


Reply via email to