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:Fixed broken link and restructuredtext warnings #204

Merged
merged 2 commits into from
May 20, 2022

Conversation

ziafazal
Copy link
Contributor

This PR has changes to fix broken link to Supports Events in documentation and fixes warning which break this command

 python setup.py check --restructuredtext --strict

resulting in failure to generate documentation.

Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

@ziafazal this fixes some of the issues, but if you look at the output of make html in /docs you'll see that there are a bunch more similar issues.

eg. A lot of the links on the Supported Events page also have the same broken link problem. I think the links need to be updated to the :doc: type links for referencing other RST files in the same book throught this project.

Totally fine to merge this as is, but it hasn't fully solved the linking issues in the docs.

@@ -37,7 +37,7 @@ Event consumers may never want to lose certain events even after a brief failure
Supported events and mapping of edx events onto xAPI and Caliper formats
------------------------------------------------------------------------

List of supported edx events can be found in `Supported_events <./event-mapping/Supported_events.rst>`_ along with their mapping onto xAPI and Caliper format.
List of supported edx events can be found in `Supported_events <https://github.com/openedx/event-routing-backends/blob/master/docs/event-mapping/Supported_events.rst>`_ along with their mapping onto xAPI and Caliper format.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
List of supported edx events can be found in `Supported_events <https://github.com/openedx/event-routing-backends/blob/master/docs/event-mapping/Supported_events.rst>`_ along with their mapping onto xAPI and Caliper format.
List of supported edx events can be found in :doc:`Supported Events <event-mapping/Supported_events>` along with their mapping onto xAPI and Caliper format.

We shouldn't link off site for documents that are in this repo, we should be able reference them directly like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we explicitly link off site here because Supported events page on github links to a lot of other json documents which are on github. If we don't link off site here we'll have to change all those links pointing to github in the Supported events page.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think the correct thing to do would be to correct all the github RST links to be relative links to stay in the sphinx docs as much as possible except for the actual samples. Having some rendered RST and some raw RST seems like a weird mix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah all RST links are relative link except this link in getting_started.rst and reason for that is it needs to point to github.com version of Supported_events.rst instead of sphinx version. Most of the links in Supported_events.rst are relative and point to samples which reside on github.com too. If we make this link relative here and point to sphinx version of Supported_events.rst then we'll have to make all samples links absolute since they are on github.com so I think keep a single link absolute(offsite) here is better than option. One more thing raw RST of Supported_events.rst looks better than rendered RST.

@feanil
Copy link
Contributor

feanil commented May 16, 2022

Note, I mean fine to merge after you accept my suggested change which will fix this link more correctly.

Fixed few more broken links
@ziafazal ziafazal merged commit 2ddda6f into master May 20, 2022
@ziafazal ziafazal deleted the ziafazal/fix-broken-link branch May 20, 2022 07:35
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.

2 participants