Output a more clear configuration error #421

snejus · 2020-07-09T18:45:19Z

I have made things!

This tackles #420; missing configuration is now reported like this:

(django-stubs-3.6.11) ➜  django-stubs issue-420 ✗ mypy -p django-stubs --config-file test-data/plugins.ini                                                                                                                                    
usage: (config)
...
[mypy.plugins.django_stubs]
    django_settings_module: str (required)
...
(django-stubs) mypy: error: 'django_settings_module' is not set: no section [mypy.plugins.django-stubs]

The function got refactored a little bit and made a little less verbose. I thought this way it will be easier to add new configuration options (a solution to #417 which is on the way for example).

Related issues

Closes Exceptions should be reported in a more clear way #420

sobolevn

Thanks a lot! Great work!

Two things:

Let's change how we work with constants here
Let's test the new output!

👍

sobolevn · 2020-07-09T18:48:28Z

mypy_django_plugin/main.py

-    return django_settings_module
+    parser.read(cast(str, config_file_path))
+
+    section = 'mypy.plugins.django-stubs'


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

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

sobolevn · 2020-07-09T18:49:29Z

mypy_django_plugin/main.py

+                       "\t(required) django_settings_module: str"])
+
+    def raise_error(handler: ErrorHandler, error_type: int) -> NoReturn:
+        messages = {1: "'django_settings_module' is not set: no mypy config file specified",


Let's make an enum from keys here. Like @unique class ErrorType(enum.IntEnum)?

Very good idea! I think enums access time is faster than dicts.

Considering that they shouldn't get evaluated until needed for an error, where should they live?

Just had a look into this and I have mixed feelings. Enums can take the following form:

class ErrorType(enum.Enum): SOME_ERROR: "some error because bad bad something" BAD_ERROR: "bad error because something" # or, say class ErrorType(enum.IntEnum): SOME_ERROR: 1 BAD_ERROR: 2

As far as I understand, the IntEnum just allows the enum to be referenced like ErrorType(1). The reason I used an int as the key initially was that it's the smallest type that would allow to reference errors without having to add those definitions to the scope before needed.

If an enum is used, this would require a call like error(ErrorType.MISSING_SECTION) or similar, i.e. they'd be in the scope before they are needed, which isn't great considering the startup time which is already rather slow and that errors won't get thrown often.

Also, currently these messages aren't used anywhere else or have any operations done to them. If we, say, had to do any comparison operations, then attaching types to them with enums would make sense; but now they're rather limited, so I'd probably leave it as it is for now, mostly due to performance reasons though.

Let me know if I'm missing out on something or you have something planned where having them as enums would make more sense. For now I've moved the error handler import to the local scope having done some python -X importtime runs - the startup is at least double as sluggish as mypy's 🐌

Also added a try/except for the file read, and using ConfigParser.read_file instead of read, since the latter is made for multiple files and does some extras that aren't required here.

snejus · 2020-07-09T21:19:59Z

Thanks! In terms of testing, would you expect it to be written in the same language as the type checks? I struggled to find any examples.

If I try removing the configuration from one of the tests now, that's what I get:

platform linux -- Python 3.6.11, pytest-5.4.3, py-1.9.0, pluggy-0.13.1 -- /home/sarunas/.pyenv/versions/3.6.11/envs/django-stubs-3.6.11/bin/python3.6
cachedir: .pytest_cache
rootdir: /home/sarunas/Documents/misc/django-stubs, inifile: pytest.ini, testpaths: ./test-data/typecheck/test_config.yml
plugins: cov-2.10.0, mypy-plugins-1.3.0
collected 3 items                                                                                                                                                                                                                                                                          

test-data/typecheck/test_config.yml::django_settings_module_missing usage: (config)
[mypy.plugins.django_stubs]
	(required) django_settings_module: str
(django-stubs) mypy: error: 'django_settings_module' is not set: setting is not provided

FAILED
test-data/typecheck/test_config.yml::pyproject_toml_config PASSED
test-data/typecheck/test_config.yml::generate_pyproject_toml_and_settings_file_from_installed_apps_key PASSED

========================================================================================================================================= FAILURES =========================================================================================================================================
______________________________________________________________________________________________________________________________ django_settings_module_missing ______________________________________________________________________________________________________________________________
/home/sarunas/Documents/misc/django-stubs/test-data/typecheck/test_config.yml:2: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Critical error occurred

Or should this be a standard python test?

snejus · 2020-07-10T05:07:17Z

Also, I didn't find any note in the readme which mentions that some features aren't supported by Django 2.2 and python 3.6. Maybe we should be a bit more clear about it?

sobolevn · 2020-07-10T07:23:43Z

@snejus yes, we must be clear about the minimal requirements.

snejus · 2020-07-10T13:09:37Z

Perfect! Sorry to distract though, what do you reckon about the above? Just realised the second comment ended up being an essay. Skip it - the main idea: at least the way I see it, using enums would add to the already slow startup time, and I struggled to find good reasons to use it - though that's just a shallow look and I can't read your mind so I'd be happy to hear a bit more about this :)

sobolevn · 2020-07-10T14:04:29Z

@snejus seems ok! Let's skip it then!

snejus · 2020-07-18T05:28:49Z

Hey, sorry for disappearing - been a bit too busy with work.

Thanks! In terms of testing, would you expect it to be written in the same language as the type checks? I struggled to find any examples.

Do you have a comment about this by any chance? As far as I understand the type checks testing lang (actually, what is it officially called? :) ) cannot handle hard failures/exits - could you confirm this?

If that's the case I'll add a usual python test for it.

sobolevn · 2020-07-18T07:29:33Z

@snejus you can try https://github.com/typeddjango/django-stubs/tree/master/test-data/typecheck

It probably should work. If not, then we can create .py test.

snejus · 2020-07-18T10:47:52Z

Had a deeper look into it - it doesn't seem like it can be done with pytest-mypy-plugins, unless there are some undocumented hacks (though none of the current test cases handle CriticalErrrors, so I'd assume it's not a thing:

============================================================ test session starts =============================================================
platform linux -- Python 3.6.11, pytest-5.4.3, py-1.9.0, pluggy-0.13.1 -- /home/sarunas/.pyenv/versions/3.6.11/envs/django-stubs-3.6.11/bin/python3.6
cachedir: .pytest_cache
rootdir: /home/sarunas/Documents/misc/django-stubs, inifile: pytest.ini, testpaths: ./test-data/typecheck/test_config.yml
plugins: cov-2.10.0, mypy-plugins-1.3.0
collected 3 items                                                                                                                            

test-data/typecheck/test_config.yml::config_missing_django_settings_module usage: (config)
[mypy.plugins.django_stubs]
	(required) django_settings_module: str
(django-stubs) mypy: error: 'django_settings_module' is not set: setting is not provided

FAILED
test-data/typecheck/test_config.yml::pyproject_toml_config PASSED
test-data/typecheck/test_config.yml::generate_pyproject_toml_and_settings_file_from_installed_apps_key PASSED

================================================================== FAILURES ==================================================================
___________________________________________________ config_missing_django_settings_module ____________________________________________________
/home/sarunas/Documents/misc/django-stubs/test-data/typecheck/test_config.yml:2: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Critical error occurred

where the test case looks like this:

-   case: config_missing_django_settings_module
    main: |
        pass
    mypy_config: |
        [mypy.plugins.django-stubs]

This kinda makes sense - the language is suited to test type checking specifically, whereas this is sort of a test case setup level error.

I guess I'll go ahead with a .py test.

snejus · 2020-07-18T10:54:09Z

I've seen you mentioned this PR in #428 - I'll cover django setup validation too.

sobolevn · 2020-07-18T11:04:24Z

@snejus thanks a lot!

sobolevn · 2020-07-18T11:05:08Z

We use snapshots for that: https://github.com/wemake-services/wemake-python-styleguide/blob/master/tests/test_formatter/test_formatter_output.py

snejus · 2020-07-18T18:39:31Z

Ah, didn't see your message until now and got it done meanwhile. Though would it not be a bit of an overkill to use snapshots in this scenario? It's an amazing tool for your use case in WPS - since the output varies a lot.

For the input validation, the template stays the same and can take 3 error messages (see the tests) - and that's basically it. If new vars get added in the future then it just needs that field name adding there - though that won't happen often I imagine.

Considering the rest of error handling, the output will look even more boring - the way I see it (do correct me if I'm wrong) is that we're simply gonna catch the exceptions and reraise it with our generic message, like mypy django stubs have failed initialisation, check a, b, c. And targeted messages for commonly thrown exceptions.

snejus · 2020-07-18T18:47:54Z

(django-stubs-3.6.11) ➜  django-stubs issue-420 ✗ mypy -p django-stubs --config-file test-data/plugins.ini                                                                                                                                    
usage: (config)
...
[mypy.plugins.django_stubs]
    django_settings_module: str (required)
...
(django-stubs) mypy: error: 'django_settings_module' is not set: no section [mypy.plugins.django-stubs]
(django-stubs-3.6.11) ➜  django-stubs issue-420 ✗ echo '\n[mypy.plugins.django-stubs]' >> test-data/plugins.ini                                                                                                                               
(django-stubs-3.6.11) ➜  django-stubs issue-420 ✗ mypy -p django-stubs --config-file test-data/plugins.ini                                                                                                                                    
usage: (config)
...
[mypy.plugins.django_stubs]
    django_settings_module: str (required)
...
(django-stubs) mypy: error: 'django_settings_module' is not set: the setting is not provided
(django-stubs-3.6.11) ➜  django-stubs issue-420 ✗ echo '    django_settings_module = hello' >> test-data/plugins.ini                                                                                                                          
(django-stubs-3.6.11) ➜  django-stubs issue-420 ✗ mypy -p django-stubs --config-file test-data/plugins.ini                                                                                                                                    
Error constructing plugin instance of NewSemanalDjangoPlugin

Traceback (most recent call last):
  File "/home/sarunas/.pyenv/versions/django-stubs-3.6.11/bin/mypy", line 11, in <module>
    sys.exit(console_entry())
  File "/home/sarunas/.pyenv/versions/3.6.11/envs/django-stubs-3.6.11/lib/python3.6/site-packages/mypy/__main__.py", line 8, in console_entry
    main(None, sys.stdout, sys.stderr)
  File "mypy/main.py", line 89, in main
  File "mypy/build.py", line 180, in build
  File "mypy/build.py", line 227, in _build
  File "mypy/build.py", line 470, in load_plugins
  File "mypy/build.py", line 448, in load_plugins_from_config
  File "/home/sarunas/Documents/misc/django-stubs/mypy_django_plugin/main.py", line 93, in __init__
    self.django_context = DjangoContext(django_settings_module)
  File "/home/sarunas/Documents/misc/django-stubs/mypy_django_plugin/django/context.py", line 88, in __init__
    apps, settings = initialize_django(self.django_settings_module)
  File "/home/sarunas/Documents/misc/django-stubs/mypy_django_plugin/django/context.py", line 70, in initialize_django
    settings._setup()
  File "/home/sarunas/.pyenv/versions/3.6.11/envs/django-stubs-3.6.11/lib/python3.6/site-packages/django/conf/__init__.py", line 63, in _setup
    self._wrapped = Settings(settings_module)
  File "/home/sarunas/.pyenv/versions/3.6.11/envs/django-stubs-3.6.11/lib/python3.6/site-packages/django/conf/__init__.py", line 142, in __init__
    mod = importlib.import_module(self.SETTINGS_MODULE)
  File "/home/sarunas/.pyenv/versions/3.6.11/lib/python3.6/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 994, in _gcd_import
  File "<frozen importlib._bootstrap>", line 971, in _find_and_load
  File "<frozen importlib._bootstrap>", line 953, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'hello'

Looks like the output is telling me what's next 😅

snejus · 2020-07-18T19:00:21Z

mypy_django_plugin/main.py

+
+        usage = '\n'.join(['(config)',
+                           '...',
+                           '[mypy.plugins.django_stubs]',


Just realised that this entire nonsense is a constant, and that the tests already define a string for it:

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

Assuming that we follow DRY, and considering that error handling is about to get extended in general, I feel like most of this exit function should move somewhere else, say, a module called errors? It could live in mypy_django_plugin/lib/ like helpers and fullnames. I thought of adding it to the helpers, but it somehow seems wrong to from helpers import exit which kills the app.

Additionally, that'd be a good start considering the rest of error handling plans.

sobolevn

LGTM

@mkurnikov ?
@kszmigiel ?

snejus · 2020-07-18T23:14:00Z

Cool. Let's finish this PR here, (unless there are any further action points that relate to input validation).

In terms of further error handling, #428 is very much related to my #417 which has some work done already, so I believe it may make more sense to continue with it there. I found that #224 also experience similar issues.

Are you by any chance aware of any other affected tickets? Would be good to source as many as possible in order to build a clear picture and to know which parts people struggle the most with/need to get most attention.

sobolevn · 2020-07-19T07:14:41Z

Are you by any chance aware of any other affected tickets?

Nope, I guess that you have found everything we had.

kszmigiel · 2020-07-21T13:47:10Z

@sobolevn LGTM 👍
@snejus great job, love it! ❤️ Does it fix #428 already or should I take care of it after merge?

snejus · 2020-07-21T13:53:19Z

Nope, not yet! Though I'm on it currently, together with #417 to handle django-configurations.

Thanks, by the way! Next step - colours ;)

snejus · 2020-08-09T03:09:49Z

Hey - just checking whether there's anything that is blocking this getting merged in? The further error handling improvements partly depend on this (finally found time to work on it), so it'd be good know what's the status.

sobolevn · 2020-08-09T08:27:28Z

Thanks a lot!

Output a more clear configuration error

582c66c

sobolevn requested changes Jul 9, 2020

View reviewed changes

snejus added 2 commits July 10, 2020 02:04

Performance related improvements in config read handling

09fc634

Check python 3.6 with travis

c044ad8

sobolevn mentioned this pull request Jul 17, 2020

ContentType issue #428

Closed

Revert the .travis.yml py36 inclusion

3a7b28e

snejus added 2 commits July 18, 2020 19:10

Added tests for input error handling

eeb5da3

Thanks isort, isorted it out

6475537

snejus commented Jul 18, 2020

View reviewed changes

Make exit() function a bit more aesthetic

b38dde2

snejus force-pushed the issue-420 branch from ffb61ee to b38dde2 Compare July 18, 2020 19:46

sobolevn approved these changes Jul 18, 2020

View reviewed changes

Single quote -> double quote docstrings

05ff4e2

Whitespace removed

1e3bf1e

sobolevn approved these changes Aug 9, 2020

View reviewed changes

sobolevn merged commit 8a64d87 into typeddjango:master Aug 9, 2020

snejus deleted the issue-420 branch December 22, 2020 05:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Output a more clear configuration error #421

Output a more clear configuration error #421

snejus commented Jul 9, 2020 •

edited

Loading

sobolevn left a comment

sobolevn Jul 9, 2020

snejus Jul 9, 2020 •

edited

Loading

sobolevn Jul 9, 2020

snejus Jul 9, 2020

snejus Jul 10, 2020

snejus Jul 10, 2020

snejus commented Jul 9, 2020

snejus commented Jul 10, 2020 •

edited

Loading

sobolevn commented Jul 10, 2020

snejus commented Jul 10, 2020

sobolevn commented Jul 10, 2020

snejus commented Jul 18, 2020 •

edited

Loading

sobolevn commented Jul 18, 2020

snejus commented Jul 18, 2020 •

edited

Loading

snejus commented Jul 18, 2020 •

edited

Loading

sobolevn commented Jul 18, 2020

sobolevn commented Jul 18, 2020

snejus commented Jul 18, 2020

snejus commented Jul 18, 2020

snejus Jul 18, 2020 •

edited

Loading

sobolevn left a comment

snejus commented Jul 18, 2020

sobolevn commented Jul 19, 2020

kszmigiel commented Jul 21, 2020

snejus commented Jul 21, 2020

snejus commented Aug 9, 2020

sobolevn commented Aug 9, 2020

Output a more clear configuration error #421

Output a more clear configuration error #421

Conversation

snejus commented Jul 9, 2020 • edited Loading

I have made things!

Related issues

sobolevn left a comment

Choose a reason for hiding this comment

sobolevn Jul 9, 2020

Choose a reason for hiding this comment

snejus Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

sobolevn Jul 9, 2020

Choose a reason for hiding this comment

snejus Jul 9, 2020

Choose a reason for hiding this comment

snejus Jul 10, 2020

Choose a reason for hiding this comment

snejus Jul 10, 2020

Choose a reason for hiding this comment

snejus commented Jul 9, 2020

snejus commented Jul 10, 2020 • edited Loading

sobolevn commented Jul 10, 2020

snejus commented Jul 10, 2020

sobolevn commented Jul 10, 2020

snejus commented Jul 18, 2020 • edited Loading

sobolevn commented Jul 18, 2020

snejus commented Jul 18, 2020 • edited Loading

snejus commented Jul 18, 2020 • edited Loading

sobolevn commented Jul 18, 2020

sobolevn commented Jul 18, 2020

snejus commented Jul 18, 2020

snejus commented Jul 18, 2020

snejus Jul 18, 2020 • edited Loading

Choose a reason for hiding this comment

sobolevn left a comment

Choose a reason for hiding this comment

snejus commented Jul 18, 2020

sobolevn commented Jul 19, 2020

kszmigiel commented Jul 21, 2020

snejus commented Jul 21, 2020

snejus commented Aug 9, 2020

sobolevn commented Aug 9, 2020

snejus commented Jul 9, 2020 •

edited

Loading

snejus Jul 9, 2020 •

edited

Loading

snejus commented Jul 10, 2020 •

edited

Loading

snejus commented Jul 18, 2020 •

edited

Loading

snejus commented Jul 18, 2020 •

edited

Loading

snejus commented Jul 18, 2020 •

edited

Loading

snejus Jul 18, 2020 •

edited

Loading