Skip to content

Output a more clear configuration error #421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Aug 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 33 additions & 19 deletions mypy_django_plugin/main.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import configparser
from functools import partial
from typing import Callable, Dict, List, Optional, Tuple
from typing import Callable, Dict, List, NoReturn, Optional, Tuple, cast

from django.db.models.fields.related import RelatedField
from mypy.errors import Errors
from mypy.nodes import MypyFile, TypeInfo
from mypy.options import Options
from mypy.plugin import (
Expand Down Expand Up @@ -52,25 +51,40 @@ def add_new_manager_base(ctx: ClassDefContext) -> None:


def extract_django_settings_module(config_file_path: Optional[str]) -> str:
errors = Errors()
if config_file_path is None:
errors.report(0, None, "'django_settings_module' is not set: no mypy config file specified")
errors.raise_error()

def exit(error_type: int) -> NoReturn:
"""Using mypy's argument parser, raise `SystemExit` to fail hard if validation fails.

Considering that the plugin's startup duration is around double as long as mypy's, this aims to
import and construct objects only when that's required - which happens once and terminates the
run. Considering that most of the runs are successful, there's no need for this to linger in the
global scope.
"""
from mypy.main import CapturableArgumentParser

usage = """(config)
...
[mypy.plugins.django_stubs]
django_settings_module: str (required)
...
""".replace("\n" + 8 * " ", "\n")
handler = CapturableArgumentParser(prog='(django-stubs) mypy', usage=usage)
messages = {1: 'mypy config file is not specified or found',
2: 'no section [mypy.plugins.django-stubs]',
3: 'the setting is not provided'}
handler.error("'django_settings_module' is not set: " + messages[error_type])

parser = configparser.ConfigParser()
parser.read(config_file_path) # type: ignore

if not parser.has_section('mypy.plugins.django-stubs'):
errors.report(0, None, "'django_settings_module' is not set: no section [mypy.plugins.django-stubs]",
file=config_file_path)
errors.raise_error()
if not parser.has_option('mypy.plugins.django-stubs', 'django_settings_module'):
errors.report(0, None, "'django_settings_module' is not set: setting is not provided",
file=config_file_path)
errors.raise_error()

django_settings_module = parser.get('mypy.plugins.django-stubs', 'django_settings_module').strip('\'"')
return django_settings_module
try:
parser.read_file(open(cast(str, config_file_path), 'r'), source=config_file_path)
except (IsADirectoryError, OSError):
exit(1)

section = 'mypy.plugins.django-stubs'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reuse this const from somewhere else? Looks like we should already have it somewhere.

Copy link
Contributor Author

@snejus snejus Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't seem like it - at least not anywhere in the source code:
image

if not parser.has_section(section):
exit(2)
settings = parser.get(section, 'django_settings_module', fallback=None) or exit(3)
return cast(str, settings).strip('\'"')


class NewSemanalDjangoPlugin(Plugin):
Expand Down
4 changes: 3 additions & 1 deletion pytest.ini
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
[pytest]
testpaths = ./test-data
testpaths =
./test-plugin
./test-data
addopts =
--tb=native
-s
Expand Down
73 changes: 73 additions & 0 deletions test-plugin/test_error_handling.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import tempfile
import typing

import pytest

from mypy_django_plugin.main import extract_django_settings_module

TEMPLATE = """usage: (config)
...
[mypy.plugins.django_stubs]
django_settings_module: str (required)
...
(django-stubs) mypy: error: 'django_settings_module' is not set: {}
"""


@pytest.mark.parametrize(
'config_file_contents,message_part',
[
pytest.param(
None,
'mypy config file is not specified or found',
id='missing-file',
),
pytest.param(
['[not-really-django-stubs]'],
'no section [mypy.plugins.django-stubs]',
id='missing-section',
),
pytest.param(
['[mypy.plugins.django-stubs]',
'\tnot_django_not_settings_module = badbadmodule'],
'the setting is not provided',
id='missing-settings-module',
),
pytest.param(
['[mypy.plugins.django-stubs]'],
'the setting is not provided',
id='no-settings-given',
),
],
)
def test_misconfiguration_handling(capsys, config_file_contents, message_part):
# type: (typing.Any, typing.List[str], str) -> None
"""Invalid configuration raises `SystemExit` with a precise error message."""
with tempfile.NamedTemporaryFile(mode='w+') as config_file:
if not config_file_contents:
config_file.close()
else:
config_file.write('\n'.join(config_file_contents).expandtabs(4))
config_file.seek(0)

with pytest.raises(SystemExit, match='2'):
extract_django_settings_module(config_file.name)

error_message = TEMPLATE.format(message_part)
assert error_message == capsys.readouterr().err


def test_correct_configuration() -> None:
"""Django settings module gets extracted given valid configuration."""
config_file_contents = [
'[mypy.plugins.django-stubs]',
'\tsome_other_setting = setting',
'\tdjango_settings_module = my.module',
]
with tempfile.NamedTemporaryFile(mode='w+') as config_file:
config_file.write('\n'.join(config_file_contents).expandtabs(4))
config_file.seek(0)

extracted = extract_django_settings_module(config_file.name)

assert extracted == 'my.module'