diff --git a/README.md b/README.md index a8c6e32..a180a50 100644 --- a/README.md +++ b/README.md @@ -107,7 +107,6 @@ output will be: ``` * TODO - - Fine-grained methods for silencing checks. - Should only warn for each unhandled Converter once. - Regex patterns perhaps? (only RoutePattern supported at the moment). diff --git a/django_urlconfchecks/check.py b/django_urlconfchecks/check.py index 8c9ccc5..106593a 100644 --- a/django_urlconfchecks/check.py +++ b/django_urlconfchecks/check.py @@ -1,4 +1,5 @@ """Quick and dirty URL checker.""" +import fnmatch import typing as t import uuid from inspect import Parameter, _empty, signature # type: ignore[attr-defined] @@ -8,9 +9,21 @@ from django.urls import URLPattern, URLResolver, converters, get_resolver from django.urls.resolvers import RoutePattern +Problem = t.Union[checks.Error, checks.Warning] + +# Update docs about default value if you change this: +_DEFAULT_SILENCED_VIEWS = { + # CBVs use **kwargs, and have a `__name__` that ends up looking like this in URLconf + "*.View.as_view..view": "W001", + # RedirectView is used in a way that makes it appear directly, and it has **kwargs + "django.views.generic.base.RedirectView": "W001", + # Django contrib views currently don’t have type annotations: + "django.contrib.*": "W003", +} + @checks.register(checks.Tags.urls) -def check_url_signatures(app_configs, **kwargs) -> t.List[t.Union[checks.Error, checks.Warning]]: +def check_url_signatures(app_configs, **kwargs) -> t.List[Problem]: """Check that all callbacks in the main urlconf have the correct signature. Args: @@ -25,15 +38,14 @@ def check_url_signatures(app_configs, **kwargs) -> t.List[t.Union[checks.Error, resolver = get_resolver() errors = [] + silencers = _build_view_silencers(getattr(settings, "URLCONFCHECKS_SILENCED_VIEWS", _DEFAULT_SILENCED_VIEWS)) for route in get_all_routes(resolver): errors.extend(check_url_args_match(route)) - return errors + return _filter_errors(errors, silencers) def get_all_routes(resolver: URLResolver) -> t.Iterable[URLPattern]: """Recursively get all routes from the resolver.""" - if resolver.app_name == 'admin': - return [] for pattern in resolver.url_patterns: if isinstance(pattern, URLResolver): yield from get_all_routes(pattern) @@ -42,7 +54,7 @@ def get_all_routes(resolver: URLResolver) -> t.Iterable[URLPattern]: yield pattern -def check_url_args_match(url_pattern: URLPattern) -> t.List[t.Union[checks.Error, checks.Warning]]: +def check_url_args_match(url_pattern: URLPattern) -> t.List[Problem]: """Check that all callbacks in the main urlconf have the correct signature.""" callback = url_pattern.callback callback_repr = f'{callback.__module__}.{callback.__qualname__}' @@ -108,7 +120,7 @@ def check_url_args_match(url_pattern: URLPattern) -> t.List[t.Union[checks.Error elif found_type == Parameter.empty: errors.append( checks.Warning( - f'Missing type annotation for parameter `{name}`, can\'t check type.', + f'View {callback_repr} missing type annotation for parameter `{name}`, can\'t check type.', obj=url_pattern, id='urlchecker.W003', ) @@ -177,3 +189,35 @@ def get_converter_output_type(converter) -> t.Union[int, str, uuid.UUID, t.Type[ return sig.return_annotation return Parameter.empty + + +class ViewSilencer: + """Utility that checks whether errors for a view or set of views should be ignored.""" + + def __init__(self, view_glob: str, errors: t.Iterable[str]): + self.view_glob = view_glob + self.errors = set(errors) + + def matches(self, error: Problem): + """Returns True if this silencer matches the given error or warning.""" + url_pattern = error.obj + if not isinstance(url_pattern, URLPattern): + # Some other error, eg. for a convertor + return False + if error.id not in self.errors: + return False + view_name = f"{url_pattern.callback.__module__}.{url_pattern.callback.__qualname__}" + if fnmatch.fnmatch(view_name, self.view_glob): + return True + return False + + +def _build_view_silencers(silenced_views: t.Dict[str, str]) -> t.List[ViewSilencer]: + return [ + ViewSilencer(view_glob=view_glob, errors=[f"urlchecker.{error}" for error in error_list.split(",")]) + for view_glob, error_list in silenced_views.items() + ] + + +def _filter_errors(errors: t.List[Problem], silencers: t.List[ViewSilencer]) -> t.List[Problem]: + return [error for error in errors if not any(silencer.matches(error) for silencer in silencers)] diff --git a/docs/usage.md b/docs/usage.md index 20ac6ec..567ed47 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -63,3 +63,42 @@ Then, add the following to your `.pre-commit-config.yaml` file: ``` Run `pre-commit run` to check all URLConfs for errors. + + +## Silencing errors and warnings + +You can silence specific errors and warnings as described in the [System check +framework +documentation](https://docs.djangoproject.com/en/stable/topics/checks/). + +For example: + +```python +SILENCED_SYSTEM_CHECKS = [ + "urlchecker.W003“, +] +``` + +However, this turns off the check for all view functions. Instead of this you +can use the `URLCONFCHECKS_SILENCED_VIEWS` setting for more fine grained +silencing. The value must be a dictionary: + +- whose keys are fully qualified dotted paths to view functions or callables, + with globbing syntax allowed, e.g. `"my_project.views.my_view"` or + `"other_project.*"` + +- whose values are comma separated lists of warning or error IDs (without the + `urlchecker` prefix), e.g. `"E001,W003”` + + +The default value of `URLCONFCHECKS_SILENCED_VIEWS` is below. If you override it +in your `settings.py`, you will probably want to include the following and add +more items: + +```python +URLCONFCHECKS_SILENCED_VIEWS = { + "*.View.as_view..view": "W001", # CBVs + "django.views.generic.base.RedirectView": "W001", + "django.contrib.*": "W003", # admin etc. +} +``` diff --git a/setup.cfg b/setup.cfg index 25ca51b..32f3bfc 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,7 +1,7 @@ [flake8] max-line-length = 120 max-complexity = 18 -ignore = E203, E266, W503 +ignore = E203, E266, W503, D107 docstring-convention = google per-file-ignores = __init__.py:F401 exclude = .git, diff --git a/tests/test_url_checker.py b/tests/test_url_checker.py index cf40f25..386120e 100644 --- a/tests/test_url_checker.py +++ b/tests/test_url_checker.py @@ -44,6 +44,30 @@ def test_incorrect_urls(): assert error_eql(errors[0], expected_error) +def test_silencing(): + """Test that fine-grained error silencing mechanisms work.""" + # Specific view silenced + with override_settings( + ROOT_URLCONF="tests.dummy_project.urls.incorrect_urls", + URLCONFCHECKS_SILENCED_VIEWS={"tests.dummy_project.views.year_archive": "E002"}, + ): + assert len(check_url_signatures(None)) == 0 + + # Glob pattern + with override_settings( + ROOT_URLCONF="tests.dummy_project.urls.incorrect_urls", + URLCONFCHECKS_SILENCED_VIEWS={"tests.dummy_project.views.*": "E002"}, + ): + assert len(check_url_signatures(None)) == 0 + + # Non-matching silencer + with override_settings( + ROOT_URLCONF="tests.dummy_project.urls.incorrect_urls", + URLCONFCHECKS_SILENCED_VIEWS={"tests.dummy_project.views.month_archive": "E002"}, + ): + assert len(check_url_signatures(None)) > 0 + + def test_all_urls_checked(): """Test that all urls are checked. @@ -69,12 +93,11 @@ def test_child_urls_checked(): def test_admin_urls_ignored(): - """Test that admin urls are ignored. + """Test that admin urls errors are ignored with default settings. Returns: None """ with override_settings(ROOT_URLCONF='tests.dummy_project.urls.admin_urls'): - resolver = get_resolver() - routes = get_all_routes(resolver) - assert len(list(routes)) == 0 + errors = check_url_signatures(None) + assert len(errors) == 0