when not serving static files, don't mess the apps static_root_url #164
Conversation
|
Code looks good, but please can you sort out your fork and get the PR right. |
|
I'm not sure what happened, I only forked today from master, don't know why fork was suddenly behind. I made another change in a TC to make some test pass. I'm not sure, if the approach of adding static_path='.' in TC is ok, what do you think? |
|
should be ok I think. Can you fix tests. |
Codecov Report
@@ Coverage Diff @@
## master #164 +/- ##
==========================================
+ Coverage 94.9% 94.91% +0.01%
==========================================
Files 12 12
Lines 765 767 +2
Branches 93 95 +2
==========================================
+ Hits 726 728 +2
Misses 25 25
Partials 14 14
Continue to review full report at Codecov.
|
|
I silenced one of the warnings with '# noqa: C901' |
aiohttp_devtools/runserver/serve.py
Outdated
| static_url = 'http://{}:{}/{}'.format(get_host(request), config.aux_port, static_path) | ||
| dft_logger.debug('settings app static_root_url to "%s"', static_url) | ||
| app['static_root_url'] = static_url | ||
| if config.static_path is not None: |
There was a problem hiding this comment.
can you remove if config.static_path is not None here and change above to
if config.infer_host and config.static_path is not None:We should also change this to use the new version of aiohttp middleware, you can either do this here or I'll do another PR
There was a problem hiding this comment.
Maybe, this middleware can be left out entirely?
It changes app['static_root_url'] on evey request, but indeed, it was already changed in the body of modify_main_app(), that is executed (at least i guess so) when setting up the app.
Also, I'm not very deep in the details of aiohttp_devtools, so maybe there is a reason to have the middleware, too. But with the quick glance I have taken, I couldn't see a reason.
|
|
||
|
|
||
| def test_modify_main_app_all_off(tmpworkdir): | ||
| mktree(tmpworkdir, SIMPLE_APP) | ||
| config = Config(app_path='app.py', livereload=False, host='foobar.com') | ||
| config = Config(app_path='app.py', livereload=False, host='foobar.com', static_path='.') |
There was a problem hiding this comment.
Afraid I haven't look at this code for a long time. Why are all these changes required?
There was a problem hiding this comment.
the default config objects have static_path=None
if static_path is None then the if block i added doesn't execute
then the app objects don't have app['static_root_url'] not set at all and the tests throw key errors
Maybe, it would be better, to add app['static_root_url'] to the app objects?
There was a problem hiding this comment.
the code change is fine, I just don't think you need to change this on every single test. Just change it on one test for full test coverage.
There was a problem hiding this comment.
the tests fail without it, because app object is missing 'static_root_url' key
tests updated changed middleware to aiohttp's newer decorator middleware
|
Also changed the middleware to new syntax EDIT: Obviously, aiohttp-2.0 doesn't have web.middleware. So should we leave it like it is? |
|
we should drop support for aiohttp 2.0.0 |
|
agree |
|
replaced by #170 as I need to get this fixed before I can fix the unquoting problem. Thanks |
I am serving the static files from within my app. the app's static_root_url get's messed up even, if I don't use --static on adev cli.
Added two ifs to only modify the app's static_root_url when the aux server is actually serving files
NB: I didn't get, why this three lines are there twice, from first look, outer should be sufficient?