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

Converted Markdown files to reStructuredText #5

Merged
merged 3 commits into from Oct 19, 2018

Conversation

apojomovsky
Copy link
Contributor

In this PR

  • All the documentation files except the README.md were converted from md to rst.
  • Links were fixed in order to be correctly parsed by Sphinx.

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

@apojomovsky Overall looks good. I wonder though if free URLs are hyperlinked or not after going through Sphinx. Worth checking.

Also, I see that many code-blocks lack a language assignment. That was the case before as well, so fixing that could be deferred.


Parameters in ROS 2 are based on services, and as such have a similar profile.
The difference is that parameters use a much larger queue depth so that requests do not get lost when, for example, the parameter client is unable to reach the parameter service server.

- System default
*
System default
Copy link
Contributor

Choose a reason for hiding this comment

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

@apojomovsky this one and immediately above, spurious newline after *?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. They don't seem to have effect in the rendered document though, but I just fixed them.


ament build . --force-cmake-configure --cmake-args -DCMAKE_BUILD_TYPE=Debug -- --ament-cmake-args -DCMAKE_BUILD_TYPE=Release

:raw-html-m2r:`<br>`
Copy link
Contributor

Choose a reason for hiding this comment

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

@apojomovsky is this really necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, thanks for pointing it out! Just removed it.

* https://docs.google.com/a/osrfoundation.org/spreadsheets/d/1OSwqbE3qPF8v3HSMr8JOaJ6r4QOiQFk6pwgaudXVE-4/edit?usp=sharing

* For machines hosted at OSRF, you'll need to be on the OSRF network or have a VPN connection.
* For machines which require :raw-html-m2r:`<ssh keys>` ask on ros@osrfoundation.org for your public keys to be added.
Copy link
Contributor

Choose a reason for hiding this comment

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

@apojomovsky here and immediately below, that raw-html role looks like the conversion tool choked at those angle brackets. I'd remove them if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, yes, it seems the converter identified it as a piece of html, not necessary though. Just removed them, thanks!

@apojomovsky
Copy link
Contributor Author

apojomovsky commented Oct 18, 2018

Thanks for the review @hidmic , just finished addressing your comments and to fix some other minor things here and there.
edit: the README.md was also converted to rst

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM! @chapulina any thoughts?

@hidmic
Copy link
Contributor

hidmic commented Oct 19, 2018

Ok, I think this one's ready.

@hidmic hidmic merged commit 07418b4 into ros2:master Oct 19, 2018
@apojomovsky apojomovsky deleted the apojomovsky/convert_md_to_rst branch October 19, 2018 17:16
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.

None yet

2 participants