Opened 6 years ago

Last modified 2 months ago

#26472 assigned New feature

Allow finer granularity in the ability to silence checks

Reported by: Peter Zsoldos Owned by: Adam Johnson
Component: Core (System checks) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Change History (7)

comment:1 Changed 6 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

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.

comment:2 Changed 6 years ago by Simon Charette

Summary: allow model field level silencing of system checks (e.g.: W342)Allow finer granularity in the ability to silence checks
Version: 1.8master

Agreed, I can see this being useful at the app, model and field level.

comment:3 Changed 6 years ago by Peter Zsoldos

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

comment:4 Changed 5 years ago 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.

comment:5 Changed 4 months ago 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.

comment:6 Changed 4 months ago by Adam Johnson

Has patch: set
Owner: changed from nobody to Adam Johnson
Status: newassigned

comment:7 Changed 2 months ago by Carlton Gibson

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top