Update config #1903
Update config #1903
Conversation
|
Please look at discusion here: (#1895) |
|
OK I will do more needed work like documentation (perhaps next week because I will be off for few days). |
|
Please review, comment, advise. |
Co-authored-by: tigerthelion <bjt.thompson@gmail.com>
sanic/config.py
Outdated
| import types | ||
| from os import environ as os_environ | ||
|
|
||
| from typing import Union, \ |
There was a problem hiding this comment.
I assume that you're using \ charachter here in order to avoid linter errors, am I right? Also, in my opinion would be better to use this format
from typing import Union, Any
# OR
from typing import (Union, Any)
There was a problem hiding this comment.
For me it just looks nicer to read.
But if You do not like it - we can do as You like.
Just decide and let me know.
There was a problem hiding this comment.
The official stance is that you should run black and isort. We do not need to make these decisions about style choice then 😆
There was a problem hiding this comment.
So the black and isort will do the job for us ?
There was a problem hiding this comment.
Yes, actually isort usually doing formatting which I've mentioned above, you need to run black/isort and it will do all necessary formatting, etc.
There was a problem hiding this comment.
There should be a command to run this in the Makefile. I usually run make fix-import which (contrary to the name) also runs black. Probably worth a fix.
There was a problem hiding this comment.
So we should not bother much about it because during "building process" it will be "blacked" and "isorted" anyway ?
There was a problem hiding this comment.
You should manually run make black and make fix-import commands.
There was a problem hiding this comment.
I runned: "make fix-import".
But it changed not only imports - please take a look.
So perhaps fix-import should only fix imports,
and there should be another target that would fix-imports and do other stuff (stuff that now fix-import does).
|
@tomaszdrozdz We want to try to get this merged in before the 2020-09 release. How are you progressing with those final changes requested last week? |
|
I found something, When I run: test_load_module_from_file_location from tests/test_load_module_from_file_location.py is run first so all tests pass. then test_update from ests/test_update_config.py pytest is skipped Can we force somehow test execution order ? I have found this: |
|
Quick tox how to please ? |
|
And apologize for being absent. |
|
Thanks, will look and try to merge tomorrow night before next week's release. |
How to what? |
Needing to run tests in a specific order sounds to me like there is a problem with the underlying tests. |
Do not quite understand what kind of problem You can think of ? Pytest by default runs test in order they are discovered - so it takes into account directories names, files names, order of test in a module. But if we change this tests files names so they switch alphabetically order we will run test_update() before test_load_module_from_file_location() so it will be skipped. Unfortunately https://pytest-ordering.readthedocs.io/en/develop/ seams to be more of "I wish to have" then actual work done. We can not to use pytest dependencies, but then we get different issue: |
|
And about tox - hot to run test with tox: |
Yes, you are almost right, but you should install it. Please, see https://sanic.readthedocs.io/en/latest/sanic/contributing.html |
Sanic is setup to run tests on 3.6, 3.7, and 3.8. You can limit it in tox to just one of them: |
|
Need any help on this? I want to merge it on today/tomorrow (2020-09-29.) |
|
Currently I work on system where highest python version is 3.5. Tu run any python version I use conda To be honest there are some efforts to make tox work with conda - but have not tried it. So please, please if You think this pull request changes are ready to merge to master As I can see in "tarvis" check
And some errors for appveyor for -no-ext python 3.6, 3.7, 3.8 - but I do not know what it means. |
|
OK. I'll take a look tonight and try and take a pass at it. |
|
@tomaszdrozdz I want to say that I think you did a bang up job on this. Thanks for all the back and forth and patience with all the comments. |
| ) | ||
| except IOError as e: | ||
| e.strerror = f"Unable to load configuration file (e.strerror)" | ||
| e.strerror = "Unable to load configuration file (e.strerror)" |
There was a problem hiding this comment.
Shouldn't it be ?:
e.strerror = "Unable to load configuration file {e.strerror}"
|
@ahopkins @myusko @ashleysommer @tigerthelion |
No description provided.