[Django] #26472: allow model field level silencing of system checks (e.g.: W342)
Django
--------------------------------------+--------------------
Reporter: zsoldosp | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
==== Proposal ====
Add an optional kwarg to modelfields called
`silenced_system_check=<iterable>`, which would default to `None` or an
empty collection
As `CheckMessage` already has access to the object it applies to, it
should be a matter of adding an extra check in `is_silenced` to check
whether the warning id is present in the global system settings or inside
the modelfield's own silenced system checks
https://github.com/django/django/blob/master/django/core/checks/messages.py#L54
==== Context ====
on a large application that was recently upgraded to 1.8, we wanted to add
a step to the build to fail is `manage.py check` would report any
warnings. However, there are many uses of
`models.ForeignKey(unique=True)`, and thus upgrading all of them right now
is not feasible. The only option we have is to use
`settings.SILENCED_SYSTEM_CHECKS`, however, that silencing the warning
there would apply to any new models/fields that are added, for which we
would actually be happy to fail the build.
--
Ticket URL: <https://code.djangoproject.com/ticket/26472>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: zsoldosp | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
Comment:
I don't think the proposed API of a model field argument is a good one
(this wouldn't allow a project to silence a warnings in third-party app,
for example), but the idea of finer granularity in the ability to silence
errors has come up before and would be great.
--
Ticket URL: <https://code.djangoproject.com/ticket/26472#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: zsoldosp | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* version: 1.8 => master
Comment:
Agreed, I can see this being useful at the app, model and field level.
--
Ticket URL: <https://code.djangoproject.com/ticket/26472#comment:2>
Django
--------------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by zsoldosp):
would a dictionary based setting make sense? E.g.: the user would
configure the following
{{{
SILENCED_SYSTEM_CHECKS = {
'__all__': ['x1234', 'y5678'],
'some.3rdparty.app': ['w1234'],
'my.models.SomeModel.field': ['w5678']
}
}}}
this of course has to be transformed into a more efficient data structure
at project startup
--
Ticket URL: <https://code.djangoproject.com/ticket/26472#comment:3>
Django
--------------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
I had an idea to use a similar syntax as flake8 which uses `# NOQA` to
ignore warnings on a particular line. For example, something like `#
silence-check: fields.W1234` could be added to a line that generates a
particular warning. That could allow third-party apps, for example, to
silence warnings without requiring a project using that app to modify its
settings. I'm not sure if implementing this idea would be feasible or
practical though.
--
Ticket URL: <https://code.djangoproject.com/ticket/26472#comment:4>
Django
--------------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam Johnson):
> would a dictionary based setting make sense?
I'm not hot on the idea of this settings approach:
1. It centralizes check silencing unnecessarily. It makes more sense to
silence a check next to the relevant defintion, where one could write a
comment about *why*.
2. Not every object has a dotted path. For example if something is
defined inside a functino closure it will have `<locals>` inside its
`__qualname__`, so one couldn't refer to it with a setting. This can come
up pretty easily, such as model classes extended by class decorators.
> I had an idea to use a similar syntax as flake8 which uses `# NOQA` to
ignore warnings on a particular line.
I'm also not in favour of a comment-based approach. It's a nice idea, but
it would require Django to re-parse and tokenize the code to find the
comments, which would significantly increase runtime cost. IMO this kind
of operation is best reserved for linters.
----
I propose adding a function to silence checks:
{{{
def silence_check(id_, obj=None):
}}}
This should be general enough since `CheckMessage.id` can be *any* object.
For objects like fields, one could call it after the definition:
{{{
class Book(models.Model):
title = ...
silence_check("models.E999", title)
}}}
It could also support decorator syntax:
{{{
@silence_check("models.E999")
class Book(models.Model):
...
}}}
...which would also allow calls like this:
{{{
class Book(models.Model):
title = silence_check(
"models.E999",
models.CharField(...),
)
}}}
The implementation can use a weak dict of objects to the set of silenced
check ID's. Using a weak dict will allow silencing to be applied to
temporary objects without causing permanent references to be held by the
checks framework.
--
Ticket URL: <https://code.djangoproject.com/ticket/26472#comment:5>
Django
Reporter: Peter Zsoldos | Owner: Adam
| Johnson
Type: New feature | Status: assigned
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam Johnson):
* owner: nobody => Adam Johnson
* status: new => assigned
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26472#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Peter Zsoldos | Owner: Adam
| Johnson
Type: New feature | Status: assigned
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26472#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Peter Zsoldos | Owner: Adam
| Johnson
Type: New feature | Status: assigned
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Adam Johnson):
I've just closed my previous PR.
I do now think a settings-based approach would probably be better and
clearer, per the arguments put forth by Simon and Carlton on the PR.
I hope to write a new version soon.
--
Ticket URL: <https://code.djangoproject.com/ticket/26472#comment:8>