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

LifeCycle_Preview created #19

Merged
merged 7 commits into from
Jul 20, 2024
Merged

LifeCycle_Preview created #19

merged 7 commits into from
Jul 20, 2024

Conversation

Avisheet
Copy link
Contributor

rmf_obstacle_detector_laserscan waiting to configure

While running ros2 run rmf_obstacle_detector_laserscan laserscan_detector , I encountered waiting to configure .
The Lifecycle_preview provides a detailed description about how to configure and activate the node.

WhatsApp Image 2024-03-16 at 4 58 31 PM

Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

Appreciate the effort to improve the documentation but there's no need to create a new markdown page summarizing lifecycle nodes. You may add a line to the existing readme that highlights that the nodes are lifecycle node and link to the lifecycle design document. Copy pasting text from existing documentation into a separate file here is not favorable practice.

It's best to add the commands to configure and activate the node right after the existing command to run the node

Also please rebase this PR with the latest main and ensure there are no unnecessary commits from other contributors.

@Avisheet
Copy link
Contributor Author

Thanks @Yadunund for reviewing and giving me suggestions . I was firstly thinking to provide a different markdown page for this with a brief description .
I will ensure to make the changes as suggested . Can you tell me should I add the picture of this "waiting to configure..." point or should I just add the commands .

@Yadunund
Copy link
Member

No need to include a screenshot.

@Avisheet Avisheet requested a review from Yadunund March 18, 2024 05:24
Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

Please rebase your PR with main and ensure you don't include commits from other authors.

README.md Outdated Show resolved Hide resolved
@Yadunund
Copy link
Member

Yadunund commented Apr 2, 2024

@Avisheet do you still plan on addressing the comments in the PR?

@Avisheet
Copy link
Contributor Author

Avisheet commented Apr 2, 2024

Yes @Yadunund . Due to some issues, I wast able to focus here . But I ensure you that I will be working on this .

@Avisheet Avisheet requested a review from Yadunund April 2, 2024 02:23
@Avisheet
Copy link
Contributor Author

Avisheet commented Apr 2, 2024

Hey @Yadunund, I have rebased my PR with the main. I request you to review my PR and tell me the desired changes.

@Avisheet Avisheet closed this Apr 2, 2024
@Avisheet Avisheet reopened this Apr 2, 2024
@Yadunund
Copy link
Member

Yadunund commented Apr 2, 2024

The first commit still has changes from a different author.

@Avisheet
Copy link
Contributor Author

Avisheet commented Apr 2, 2024

Hey @Yadunund, should I also remove commits 5,6 and 7?

@Avisheet
Copy link
Contributor Author

Avisheet commented Apr 2, 2024

Hey @Yadunund , the last two commits appeard while resolving the merge errors after removing commits from other authors , can you once review it and tell me the desired changes I need to do now.

@Yadunund
Copy link
Member

Yadunund commented Apr 2, 2024

Please resolve conflicts with main

Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

Thanks for iterating and sorry for the delay in getting this merged.

@Yadunund Yadunund merged commit a0fc2cd into open-rmf:main Jul 20, 2024
1 check passed
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