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

Update URLs #2881

Merged
merged 1 commit into from
Dec 1, 2023
Merged

Update URLs #2881

merged 1 commit into from
Dec 1, 2023

Conversation

xuanxu
Copy link
Member

@xuanxu xuanxu commented Nov 16, 2023

This PR updates URLs in docs and code comments to use https and/or updating the URL if the original one got a redirect as response.

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (56ee8f2) 66.52% compared to head (4d23feb) 66.52%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2881   +/-   ##
=======================================
  Coverage   66.52%   66.52%           
=======================================
  Files         235      235           
  Lines       18101    18101           
=======================================
  Hits        12041    12041           
  Misses       6060     6060           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@keflavich
Copy link
Contributor

This looks reasonable to me, but @xuanxu could you clarify which URLs were redirects? I see at least the atomic line list one on a quick browse, but not sure about the others.

@xuanxu
Copy link
Member Author

xuanxu commented Nov 16, 2023

yeah, there's a couple of visible cases like that: www.numpy.org going to numpy.org and docs.python-requests.org/en/master/ to docs.python-requests.org/en/latest, and even one URL that is responding a 404: http://continuum.io

But I was just doing a general reference, considering also the URLs that redirects with a 301 code to the https version, like www.astropy.org for example.

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

LGTM but I'll await @bsipocz 's final judgement.

@bsipocz bsipocz added this to the v0.4.7 milestone Nov 16, 2023
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

Looks good, but I leave it open until tomorrow and check back whether you would be interested in pushing another commit that more systematically fixes the redirected/broken links.

@xuanxu
Copy link
Member Author

xuanxu commented Nov 17, 2023

Looks good, but I leave it open until tomorrow and check back whether you would be interested in pushing another commit that more systematically fixes the redirected/broken links.

That was my first goal, but I realized halfway that the rest of http URLs are part of the code in the form of configurations, internal calls, tests and data files, and updating them to the https version is not trivial task and I'm not sure it won't break backwards compatibility.

For example: every call to the http://www.ivoa.net domain now redirects to its https version, but the XML schemas you get after the redirection reference the old URL. See the case of http://www.ivoa.net/xml/VOTable/v1.3 a namespace value used in multiple XML files in astroquery modules. It redirecs to https://www.ivoa.net/xml/VOTable/VOTable-1.4.xsd but referencing the original http version in its targetNamespacevalue. Do we update that namespace value in our XMLs to the https version? to the new url? or we keep the http version used in the result? It looks like it may not be a good idea to update it, but I'm not sure.

I mean, I consider this PR a good systematic fixing of broken/redirected links in safe URLs. The ones in the code make for a separate PR, it's a task needing more research in a case by case basis (I'll add it to my to do list).

@bsipocz
Copy link
Member

bsipocz commented Nov 17, 2023

Sounds good. In the meantime I fixed the tox linkcheck command, so those weekly cron run logs can be used

@bsipocz bsipocz merged commit d828534 into astropy:main Dec 1, 2023
10 of 11 checks passed
@bsipocz
Copy link
Member

bsipocz commented Dec 1, 2023

Thanks @xuanxu!

@xuanxu xuanxu deleted the avoid-url-redirects branch December 1, 2023 09:04
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.

None yet

3 participants