Skip to content

Add first_rclrs_node example and update documentation links #467

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

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

roboticswithjulia
Copy link

Merge Request: Add Improvements to Documentation in first_rclrs_node.rs

Description

This merge request moves the previous documentation written in docs/writing-your-first-rclrs-node.md into a custom example documentation ./examples/minimal_pub_sub/src/first_rclrs_node.rs. The changes aim to enhance clarity, readability and integration with Rust ecosystem.

Changes

  • Code has been updated with the latest changes in the rclrs crate, corresponding with the current repository.
  • Documentation has been added following the Missing functions in rclrs #465 (comment) indication. Thank you @mxgrey!
  • New node has been added in minimal_pub_sub, which allows also to compile the example.

Modified Files

  • writing-your-first-rclrs-node.md [Deleted]
  • first_rclrs_node.rs
  • Readme.md

How to Test

  1. Review the changes in the first_rclrs_node.rs file.
  2. Ensure that the instructions are clear and easy to follow.

Additional Notes

No functional code changes were made; only documentation improvements were included in this merge request.


Please review the changes and provide feedback or approval.

@esteve
Copy link
Collaborator

esteve commented Mar 26, 2025

@roboticswithjulia thanks for submitting this PR. It looks like the code is not properly formatted, CI fails at the cargo fmt step:

  /usr/bin/bash -c source /opt/ros/humble/setup.sh && colcon test-result --verbose
  build/examples_rclrs_minimal_pub_sub/cargo_test.xml: 2 tests, 0 errors, 1 failure, 0 skipped
  - fmt
    <<< failure message
      cargo fmt failed
    >>>
  
  Summary: 10 tests, 0 errors, 1 failure, 0 skipped
Error: The process '/usr/bin/bash' failed with exit code 1

Can you make sure that the new file is formatted according to the Rust styleguide? (cargo fmt --help will give you instructions on how to use cargo fmt)

@esteve
Copy link
Collaborator

esteve commented Mar 26, 2025

I found https://github.com/marketplace/actions/markdown-embed-code-from-file, which helps keeping Markdown and code files in sync, it'd be nice to keep the Markdown file we already have and just make sure that both the embedded example and the source file were in sync.

@roboticswithjulia
Copy link
Author

roboticswithjulia commented Mar 26, 2025

I found https://github.com/marketplace/actions/markdown-embed-code-from-file, which helps keeping Markdown and code files in sync, it'd be nice to keep the Markdown file we already have and just make sure that both the embedded example and the source file were in sync.

I think that's a good idea as it would be aligned with what it was before. I can create a new crate just for this specific example and add there the Readme.md and the code itself aligned on the link explanation.

But, maybe I misunderstood something when I was reading through the @mxgrey comment #465 (comment), and as far as I understood the idea was to document the crate itself to be compatible with the cargo doc --open and have this example also be compatible with the Rust docu. This is how it looks like with what this PR provides, but I am open to change anything that could be helpful :).
image.
Thank you for your help @esteve !

@roboticswithjulia
Copy link
Author

roboticswithjulia commented Mar 26, 2025

@roboticswithjulia thanks for submitting this PR. It looks like the code is not properly formatted, CI fails at the cargo fmt step:

  /usr/bin/bash -c source /opt/ros/humble/setup.sh && colcon test-result --verbose
  build/examples_rclrs_minimal_pub_sub/cargo_test.xml: 2 tests, 0 errors, 1 failure, 0 skipped
  - fmt
    <<< failure message
      cargo fmt failed
    >>>
  
  Summary: 10 tests, 0 errors, 1 failure, 0 skipped
Error: The process '/usr/bin/bash' failed with exit code 1

Can you make sure that the new file is formatted according to the Rust styleguide? (cargo fmt --help will give you instructions on how to use cargo fmt)

I am surprised how just leaving few unneeded left spaces in the comments can make a pipeline fail! Just removed few spaces and now pipeline works :) Thank you so much for the feedback!

@roboticswithjulia
Copy link
Author

roboticswithjulia commented Apr 1, 2025

Hello @esteve and @mxgrey,
I hope this message finds you well. I understand you both have busy schedules, but I wanted to respectfully follow up regarding the PR I've been working on. If you could provide some guidance on how to proceed, I'd be able to complete the necessary changes and prepare it for merging.
I'm eager to finalize this task so that I can free up bandwidth to assist with other tasks.
Thank you for your time and consideration. I appreciate your support.

@mxgrey
Copy link
Collaborator

mxgrey commented Apr 1, 2025

I'm not sure how @esteve feels about this, but I was actually thinking the tutorial could go inside rclrs/src/lib.rs, that way it's right on the landing page for the documentation of rclrs. Or alternatively maybe it could go inside rclrs/src/node.rs since it's describing how to create a basic node.

I'm concerned that since it's in an example crate right now it won't have very good visibility for new users. I'm not sure if we even publish the example crates to https://crates.io ... if we don't then this tutorial will never be generated into online documentation.

@roboticswithjulia
Copy link
Author

roboticswithjulia commented Apr 2, 2025

Hello @mxgrey ,
Thank you for your quick response! I'm completely open to relocating the current example to either rclrs/src/lib.rs or rclrs/src/node.rs if needed.
Let's wait for @esteve's feedback before making a final decision on the best path forward. Thank you!!

@esteve
Copy link
Collaborator

esteve commented Apr 2, 2025

I agree with @mxgrey, we're not publishing the examples to crates.io, so the documentation wouldn't be available online. I've tried running the tokusumi/markdown-embed-code action to embed files into Markdown (see #469), but couldn't get it to work.

@roboticswithjulia our best bet is probably to just move the tutorials to rclrs/src/lib.rs, some of the code in the tutorial does not compile (on purpose, to teach users how to fix the issues), you'd need to ignore those code blocks. Also feel free to give #469 a try if you want to see if you can make it work.

@roboticswithjulia
Copy link
Author

Yes, sure!! Let's proceed in this way then :)! I will also give a try to #469! Thank you @esteve for your help and assistance!!

esteve and others added 17 commits April 2, 2025 18:49
…rkdown files and example code

Signed-off-by: Esteve Fernandez <[email protected]>
Signed-off-by: Esteve Fernandez <[email protected]>
Signed-off-by: Esteve Fernandez <[email protected]>
Signed-off-by: Esteve Fernandez <[email protected]>
@roboticswithjulia
Copy link
Author

roboticswithjulia commented Apr 3, 2025

Hello @esteve and @mxgrey ,
I've made PR #469 work by using the markdown-autodocs package instead of markdown-embed-code. I switched packages because markdown-autodocs has more stars, is more widely used, and appears to be better maintained. I tried markdown-embed-code first but couldn't get the basic example working.

Issues to Discuss Before Merging

  1. Code Example and Warnings
When adding the example directly to [rclrs/src/lib.rs](https://github.com/ros2-rust/ros2_rust/blob/main/rclrs/src/lib.rs), I encountered several "dead code" warnings during build:
Copywarning: associated items `new` and `republish` are never used
   --> src/lib.rs:84:8
    |
83  | impl RepublisherNode {
    | -------------------- associated items in this implementation
84  |     fn new(executor: &Executor) -> Result<Self, RclrsError> {
    |        ^^^
...
103 |     fn republish(&self) -> Result<(), RclrsError> {
    |        ^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default
I worked around this by adding underscores to the structure name and its methods (new and publish). Do you know of a better approach to eliminate these warnings?
  1. Pipeline Output Issues
    The pipeline runs successfully, but I can't view the action output properly:
CopyRun dineshsonachalam/[email protected]
Run wget https://raw.githubusercontent.com/dineshsonachalam/markdown-autodocs/master/action.py
--2025-04-03 16:24:48--  https://raw.githubusercontent.com/dineshsonachalam/markdown-autodocs/master/action.py
Resolving raw.githubusercontent.com (raw.githubusercontent.com)... 185.199.108.133, 185.199.110.133, 185.199.109.133, ...
Connecting to raw.githubusercontent.com (raw.githubusercontent.com)|185.199.108.133|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 3579 (3.5K) [text/plain]
Saving to: 'action.py'

     0K ...                                                   100% 57.2M=0s

2025-04-03 16:24:48 (57.2 MB/s) - 'action.py' saved [3579/3579]

Run python3 action.py -repo 'ros2-rust/ros2_rust' -access_token '***' -commit_author 'roboticswithjulia <[email protected]>' -commit_user_email '[email protected]' -commit_message 'Synchronizing Markdown files' -branch 'main' -output_file_paths '[./docs/writing-your-first-rclrs-node.md]' -categories '[code-block]'
sh: 1: docker: not found
sh: 1: docker: not found
fatal: not in a git directory
fatal: not in a git directory
fatal: detected dubious ownership in repository at '/__w/ros2_rust/ros2_rust'
To add an exception for this directory, call:

	git config --global --add safe.directory /__w/ros2_rust/ros2_rust
fatal: detected dubious ownership in repository at '/__w/ros2_rust/ros2_rust'
To add an exception for this directory, call:

	git config --global --add safe.directory /__w/ros2_rust/ros2_rust
fatal: detected dubious ownership in repository at '/__w/ros2_rust/ros2_rust'
To add an exception for this directory, call:

	git config --global --add safe.directory /__w/ros2_rust/ros2_rust
git config user.name 'roboticswithjulia <[email protected]>'
git config user.email '[email protected]'
git add ./docs/writing-your-first-rclrs-node.md
git commit -m 'Synchronizing Markdown files' ./docs/writing-your-first-rclrs-node.md
git push origin main

Do you know how I can check the commit git commit -m 'Synchronizing Markdown files' ./docs/writing-your-first-rclrs-node.md and verify that the Markdown file was built correctly?
I've also moved the first_rclrs_node_example.rs previously created to rclrs/src/lib.rs which should resolve the issue.

  1. Dependency Concerns
    I had to add the std_msgs as a dependency in the rclrs crate, as it was needed by the example itself. I'm concerned about whether base crates should depend on other ROS packages, as this could potentially cause cyclic dependency issues in the future. What are your thoughts on this approach?

Thank you for your help, and I apologize for the multiple action execution notifications!

@roboticswithjulia
Copy link
Author

Hello @esteve and @mxgrey,
Hope I'm not catching you during time off! When you get a chance, would you mind taking a look at this MR? Just want to get your thoughts on how to move forward.
Thank you for your help,

Júlia

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