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

Handle trailing slash on http urls #10631

Merged
merged 4 commits into from
Aug 7, 2020
Merged

Handle trailing slash on http urls #10631

merged 4 commits into from
Aug 7, 2020

Conversation

aarchiba
Copy link
Contributor

@aarchiba aarchiba commented Aug 6, 2020

Description

This pull request is to ensure the cache mechanism treats http://example.com and http://example.com/ equally.

Fixes #10630

EDIT: Follow up of #10437

@pep8speaks
Copy link

pep8speaks commented Aug 6, 2020

Hello @aarchiba 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2020-08-07 14:31:36 UTC

@astropy-bot astropy-bot bot added the utils label Aug 6, 2020
@@ -2086,3 +2086,28 @@ def test_clear_download_cache_variants(temp_cache, valid_urls):
def test_ftp_tls_auto(temp_cache):
url = "ftp://anonymous:mail%40astropy.org@gdc.cddis.eosdis.nasa.gov/pub/products/iers/finals2000A.all"
download_file(url)


@pytest.mark.parametrize('base', ["http://exmaple.com", "https://example.com"])
Copy link
Member

Choose a reason for hiding this comment

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

Is the typo intentional?

Suggested change
@pytest.mark.parametrize('base', ["http://exmaple.com", "https://example.com"])
@pytest.mark.parametrize('base', ["http://example.com", "https://example.com"])

@pllim pllim added Bug Affects-dev PRs and issues that do not impact an existing Astropy release labels Aug 6, 2020
@pllim pllim added this to the v4.0.2 milestone Aug 6, 2020
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix! The diff looks fine, but I'll play with this tomorrow.

@pllim
Copy link
Member

pllim commented Aug 6, 2020

p.s. Ignore coverage failure as usual.

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Seems to work. Thanks!

If you don't care to fix the typo, this should be okay to merge. The typo doesn't really affect the test, but I'll wait for your reply.

@aarchiba
Copy link
Contributor Author

aarchiba commented Aug 7, 2020

Er, not sure whether this wants a changelog entry? Otherwise fine.

@pllim pllim removed the Affects-dev PRs and issues that do not impact an existing Astropy release label Aug 7, 2020
@pllim
Copy link
Member

pllim commented Aug 7, 2020

Doesn't hurt to have the change log if you have already written it. Thanks!

@pllim pllim merged commit 6662090 into astropy:master Aug 7, 2020
astrofrog pushed a commit that referenced this pull request Oct 9, 2020
Handle trailing slash on http urls
eteq pushed a commit to eteq/astropy that referenced this pull request Oct 12, 2020
Handle trailing slash on http urls
eteq pushed a commit that referenced this pull request Oct 13, 2020
Handle trailing slash on http urls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clear_download_cache is sensitive to trailing slash
3 participants