Skip to content
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

Fix static location in index when prefix is used #1662

Merged
merged 2 commits into from
May 14, 2017

Conversation

lovebug356
Copy link
Contributor

@lovebug356 lovebug356 commented Feb 19, 2017

When the prefix is used (e.g. not /), the links generated in the
index page are wrong. In this patch the relative path is correctly
calculated by the pathlib module and a absolute url is used as link
in the index page.

The original bug can be demonstrated with following example:

import pathlib

from aiohttp import web

app = web.Application()
app.router.add_static('/static', pathlib.Path(__file__).parent,
show_index=True)

web.run_app(app)

@lovebug356 lovebug356 force-pushed the static-url branch 2 times, most recently from 3ade2d0 to 558bca6 Compare February 19, 2017 21:19
When the prefix is used (e.g. not /), the links generated in the
index page are wrong. In this patch the relative path is correctly
calculated by the pathlib module and a absolute url is used as link
in the index page.

The original bug can be demonstrated with following example:

---
import pathlib

from aiohttp import web

app = web.Application()
app.router.add_static('/static', pathlib.Path(__file__).parent,
show_index=True)

web.run_app(app)

---
@codecov-io
Copy link

Codecov Report

Merging #1662 into master will decrease coverage by -0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1662      +/-   ##
==========================================
- Coverage   96.86%   96.86%   -0.01%     
==========================================
  Files          34       34              
  Lines        7302     7301       -1     
  Branches     1265     1265              
==========================================
- Hits         7073     7072       -1     
  Misses        137      137              
  Partials       92       92
Impacted Files Coverage Δ
aiohttp/web_urldispatcher.py 99.41% <100%> (-0.01%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d9053d...518bcbb. Read the comment docs.

@fafhrd91
Copy link
Member

@asvetlov could you review this PR

@AraHaan
Copy link
Contributor

AraHaan commented Feb 20, 2017

Well looks good other than the posix part because it would probably jack with running aiohttp on windows?

Like I am not sure how it would interfere with how the \'s are on Windows however it should support both / and \.

@AraHaan
Copy link
Contributor

AraHaan commented Feb 23, 2017

Mind rebasing this PR, or reopen it?

@fafhrd91
Copy link
Member

could you open PR against https://github.com/aio-libs/aiohttp

@fafhrd91 fafhrd91 force-pushed the master branch 4 times, most recently from 7c97b03 to 37c97b8 Compare March 20, 2017 19:46
@asvetlov asvetlov self-assigned this Apr 9, 2017
@AraHaan
Copy link
Contributor

AraHaan commented May 12, 2017

Seems that windows also accepts posix paths to an extent as well.

@asvetlov asvetlov merged commit 43d8bf1 into aio-libs:master May 14, 2017
@asvetlov
Copy link
Member

Thanks!

@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants