Hi Christopher, On Wed, Aug 14, 2013 at 11:26 PM, Christopher Medrela < chris.medr...@gmail.com> wrote:
> *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. > This one is entirely understandable; it's a complex area, but it's an area that isn't currently being validated. If someone wants to contribute validation for this once this GSoC project is over, it would make a nice contribution. It would be helpful if you log this sort of known omission as a bug, and flag the bug as an "easy pickings" -- that means when we get to a sprint and a developer who is new to Django contribution is looking for something to do, we can point them at this problem and suggest they take a look. > 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 > > Same goes for these three -- if you can raise a ticket for these testing omissions, we can get someone else to look at them at a later date. > *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', > ] > I'd suggest SILENCED_SYSTEM_CHECKS here -- in your example here you've already used SILENCED_WARNINGS and SILENCED_ERRORS, which points out the potential for confusion. Including the full name is also helpful, just to make clear that we're not silencing HTTP500s (other any other errors) here. As for the numbering -- I'd be inclined to use "myapp.W123", not the other way around -- put the namespace in front of the error code. 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). > I'd be inclined to use command line arguments for the tags here -- you don't want to have ambiguity between the "mytag" tag and the mytag app. ./manage.py check mytag --check=mytag --check=upgrade --check=security (not completely sure I like the --check name itself, but I hope the point is clear) *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. > I disagree -- We may not *currently* have a use for CRITICAL or INFO, but I can see how they might be useful . For example, a missing ALLOWED_HOSTS setting isn't just an error - it's a serious problem that needs to be addressed. It's worth drawing *major* attention to problems of this nature. I can see how INFO might also be useful for advisories -- for example, warning someone that a particular combination of settings may lead to poor performance (e.g., db_index turned off on a foreign key). Even if there wasn't a need for these levels, the basic concept behind the error framework strikes me as "close enough" to logging and messages that there's an implicit documentation benefit from following the same API design. > 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. > My objection to "system checks" isn't as strong as Preston's; I could certainly live with "startup checks" as a name. There's a point at which this becomes a bikeshed argument, though. > 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. > I can see the problem you're referring to, but in practice, I don't think it will be that big an issue. The problem you describe is only an issue where: * An app has checks * The app library has been imported somewhere * The app *isn't* in INSTALLED_APPS I'm having difficulty thinking of a situation where this is a problem in practice. In the case of auth, I can't think of any use case that would allow contrib.auth to be imported, but NOT be in INSTALLED_APPS -- if you're using *any* of auth -- even if you're using a custom User model -- you need to have auth in INSTALLED_APPS. Looking slightly longer term, once App Refactor lands, registering checks is something that we can do as part of app configuration, which will remove the problem entirely. The specific problem with the admin_scripts test strikes me as something that is a problem with the test, not the concept. 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. > How about using repr instead of text_type: >>> repr(1) '1' >>> repr('1') "'1'" >>> repr(u'1') "u'1'" >>> repr(None) 'None' >>> repr('None') "'None'" > 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 > Why not just have 2 tests -- a Python 2 test and a Python 3 test. Depending on the internal complexity, you can either use @skipIf decorators, or a "if sys.version" check internal to the test - I suspect the latter will probably be more appropriate. > 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 > I'm fairly certain you're correct on this. > 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 > > It's more than one element when you have a Generic Foreign Key, or any other 'combined' key. So, this is checking that your FK is pointing at something that is actually unique *and* can be the target of a foreign key. > 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 > I'm not sure I understand -- get_field() will also return fields from an inherited model. This may point to a missing test -- a check that both the fields in a unique_together or index_together are local, non-m2m fields. 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 > Hrm. I don't really like this, but I'm not sure I have a better option. A better approach would be to have GFKs turn up in get_fields, but it isn't your responsibility to fix the internal problems of Generic Foreign Keys. If we have to live with this, then we should at the very least document it as a FIXME, pointing at the underlying problem with _meta handling of GFKs. > *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. > Sounds good to me! Yours Russ Magee %-) -- 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.
