Hi Christopher, On Tue, Sep 3, 2013 at 9:25 PM, Christopher Medrela <chris.medr...@gmail.com > wrote:
> 1. Progress: I've made improvements to admin checks. I've also finished > implementing filtering and silencing checks. I've rebased my branch against > master again. > > Excellent - I'll take look and get you some feedback. > 2. I'm afraid that there is too little time to merge django-secure. > September 16 is > suggested 'pencils down' date, so there are only less than two weeks (12 > days) + > one buffer week, because firm 'pencils down' date is September 23. Merging > django-secure in such little time is actually impossible -- we must > through out > this task from schedule. I really apologize for that. > > This is obviously disappointing, but it's better to deliver something complete than a half-attempt. If we can get the core framework into a good state, merging django-secure is a self-contained task that we can address as a follow-up commit. Also -- the GSoC will come to an end, but that doesn't mean your contributions to Django have to… :-) > My plan is that this week I will work at documentation (at this moment > there is > only a draft). I will also try to implement as much "nice to have" things > as > possible. These are: > > - Writing tests which would be nice to have. I mean three tests: > - [#20974], > - test for raising error when ImageField is in use but Pillow is > not installed, > - test for raising error in `BaseCommand.__init__`. > - Moving out checks performed in `__init__`. > - Checking clashes between accessors and GenericForeignKeys. > - Checks for grouped `choices`. > > The second week and the backup week is for deep review. Regarding the > amount > of comments I got during the first review, I guess I need *at least* one > week > for deep review. > This sounds like a reasonable plan -- it's a bunch of relatively easy individual tests, each of which will individually improve the current situation, but each of which could also be omitted if time becomes a factor. When you're ready for another deep review, let me know and I'll run over the code again. [#20974] https://code.djangoproject.com/ticket/20974 > > 3. As I said, I've implemented both filtering and silencing system checks. > > You can run only a subset of checks like that: > > ./manage.py check auth -t tag1 --tag tag2 > > This command will run only these checks which are labeled with `tag1` or > `tag2` > tags and only `auth` app will be validated. If there is no app name, all > apps > are checked. If there is no tag name, all checks are performed. > > Your check function can be labeled in this way: > > from django.core.checks import register, tag > > @tag('mytag') > def my_check_function(apps, **kwargs): > # apps is a list of apps that should be validated; if None, all > apps > # should be checked. > return [] > > register(my_check_function) > > The `tag` decorator works only for entry-point functions, e.g. these one > passed > to `register` function. It doesn't work for checks > functions/methods/classmethods which are called by entry-point functions > (directly or indirectly). > I've just bounced this off Aymeric and he pointed out something interesting. We have a precedent for this in another area -- template tags. You can use a decorator to register a template tag, and that register decorator can also take arguments to control tag naming, etc. Once he pointed this out, it seemed obvious that this is what we should be doing here, too -- after all, the use case is almost identical (registering a function with a central repository, with some optional arguments associated with the registration. So - instead of having a @tag decorator, we should be modifying register to be a decorator, taking optional arguments that lets you specify tags. Depending on how easy the internals for this work, we could do either: @register('mytag') @register('othertag') def my_check_function(apps, **kwargs): … or @register('mytag', 'othertag') def my_check_function(apps, **kwargs): … And, of course, if you don't want to use decorator syntax, you can invoke the decorator directly. To silence a specific error/warning, you need to append its `id` to > SILENCED_SYSTEM_CHECKS setting. The `id` could be any string, but we use > the > following convension: "E001" for core errors, "W002" for core warnings, > "applabel.E001" for errors raised by an custom app (including contrib > apps, i.e. > "sites.E001"). The error/warning number is unique to an app, e.g. > "sites.E001", > "admin.E001" and "E001" are all allowed, but "E001" and "W001" are not OK. > You > should use "E001" and "W002". To create an error/warning with given id, > pass it > as a keyword argument: > > Error('Message', hint=None, obj=invalid_object, id='myapp.E001') > Looks good to me. > 4. More important changes in code: > > - Introduced RAW_SETTINGS_MODULE [1]. I use it in compatibility checks to > test > if `TEST_RUNNER` setting was overriden [2]. > Have a look at the internals of the diffsettings management command -- I'm not sure RAW_SETTINGS_MODULE is needed. > - Introduced `BaseAppCache.get_models_from_apps` method [3]. This method > returns > all models of given set of apps (or of all apps if None is passed) and > is used > in `default_checks.py` [4]. > I'm not sure I follow why this is needed -- or why it isn't just duplicating functionality from loading.get_models()? > - Admin changes are backward compatible. Admin validators are deprecated in > favour of admin checks, i.e. `BaseValidator` is deprecated in favour of > `BaseModelAdminChecks`. `BaseValidator` is an almost empty class now. > `ModelAdmin.validator` class attribute is deprecated in favour of new > `checks` > attribute. If an ModelAdmin defines `validator`, we are not ignoring it. > > [1] > https://github.com/chrismedrela/django/blob/gsoc2013-checks/django/conf/__init__.py#L139 > [2] > https://github.com/chrismedrela/django/blob/gsoc2013-checks/django/core/checks/default_checks.py#L33 > [3] > https://github.com/chrismedrela/django/blob/gsoc2013-checks/django/db/models/loading.py#L296 > [4] > https://github.com/chrismedrela/django/blob/gsoc2013-checks/django/core/checks/default_checks.py > > Looks good on first inspection. 5. I've left one comment on the pull request. (I mean this one about moving > registering admin checks to `autodiscover`.) > I've commented on the PR. 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.
