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

Fixes broken links (#152) #176

Closed

Conversation

pratyakshajha
Copy link
Contributor

Fix broken links listed in issue #152. I used pytest-check-links to automatically find broken links and fixed them as well.

Draft request currently as part of 152. I will need to investigate on configuring Travis CI support for this. I am assuming adding the pytest checklinks command to the build should work. I'll try to add CI support and update this PR.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@mrocklin
Copy link
Member

Thank you @pratyakshajha

Many of these fixes look good. Some of them we may not want. For example I recommend continuing to link to /latest docs. Also, I we need to keep ../proxy/.. links. They are helpful for people who run these notebooks live.

@pratyakshajha
Copy link
Contributor Author

Thanks @mrocklin.
I will fix the links to pint to /latest doc links.
I was unsure about what to do with ../proxy/.. links but this makes more sense. I will fix those.

Also, Pytest breaks for these ../proxy/.. links. I have tried to find some way to exclude it by some link patterns, but haven't found anything useful. Sphinx has an option to exclude some URL patterns while checking links but it can be slow, needs all notebooks running without errors (I had some trouble with fbprophet notebook) and lesser coverage as it can hit rate limits for some websites like Github.
Sphinx can be used instead of Pytest if we really want to integrate link checking in the CI pipeline. Or, as an alternative, Pytest can be used with a suggestion to run it maunally in CONTRIBUTING.md or maybe somewhere contributors are aware about it. I will need some suggestion on how handle this and if this is needed as a part of this PR.

Base automatically changed from master to main January 27, 2021 16:07
@jacobtomlinson
Copy link
Member

There's not been much activity here for a while. @pratyakshajha do you have any plans to continue working here?

@pratyakshajha
Copy link
Contributor Author

@jacobtomlinson I would like to finish this up

@pratyakshajha
Copy link
Contributor Author

Using #202 instead, so closing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants