Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Improve check_links.py CI script #3141

Merged
merged 4 commits into from Sep 6, 2019
Merged

Improve check_links.py CI script #3141

merged 4 commits into from Sep 6, 2019

Conversation

epwalsh
Copy link
Member

@epwalsh epwalsh commented Aug 12, 2019

It looks like this check script isn't used in the CI pipeline anymore, but I have a similar script in some of my other projects that I recently made improvements to, so I thought I'd pass them along in case we use it again (and by the way, it looks there are about 15 broken links, most of them local links from tutorials to files that no longer exist).

The updates to the script include:

  • using a single http session to make all requests, which speeds up the script by an order of magnitude.
  • using a HEAD request instead of a GET request to avoid downloading content, which also provides a significant speed-up.
  • an improved way of checking whether an external link is valid.

Copy link
Contributor

@bryant1410 bryant1410 left a comment

Choose a reason for hiding this comment

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

Maybe it'd be good if maintainers re-enable link checking and test it before merging this.

@DeNeutoy
Copy link
Contributor

Thanks, I think we had some concern about making too many requests from our CI domain or something - I think @schmmd had the most context on this.

@DeNeutoy DeNeutoy requested a review from schmmd August 15, 2019 15:52
@schmmd
Copy link
Member

schmmd commented Sep 6, 2019

@epwalsh thanks! Sorry for the delay. We don't run this as part of CI because we don't want to be spamming the internet automatically, but this is a great improvement for running it locally quickly.

@schmmd schmmd merged commit 4625a9d into allenai:master Sep 6, 2019
@epwalsh
Copy link
Member Author

epwalsh commented Sep 6, 2019

@schmmd no problem! Makes sense

@epwalsh epwalsh deleted the check-lins branch September 6, 2019 15:44
reiyw pushed a commit to reiyw/allennlp that referenced this pull request Nov 12, 2019
* improve check_links script

* fix http session initialization
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants