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.

Sign up for GitHub

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

Jump to bottom

Update Launch-system.rst #4068

Merged
merged 7 commits into from
Dec 20, 2023
Merged

Update Launch-system.rst #4068

merged 7 commits into from
Dec 20, 2023

Conversation

ashfaq92
Copy link
Contributor

added comments to the code.
I think the it is better if we tell beginner what's changed, especially the addition of

import os
from glob import glob

Otherwise, a beginner might not notice this and will take some time to figure out what's not working (happened to me).

Other part of documentation followed this. For example: https://docs.ros.org/en/humble/Tutorials/Intermediate/Tf2/Writing-A-Tf2-Broadcaster-Py.html#update-setup-py

@ashfaq92
Update Launch-system.rst
e6f2e11 
added comments to the code.
I think the it is better if we tell beginner what's changed, especially the addition of 
```
import os
from glob import glob
```
Otherwise, a beginner might not notice this and will take some time to figure out what's not working (happened to me).

Other part of documentation followed this. For example: 
https://docs.ros.org/en/humble/Tutorials/Intermediate/Tf2/Writing-A-Tf2-Broadcaster-Py.html#update-setup-py

Signed-off-by: Muhammad Ashfaq <[email protected]>
ashfaq92 and others added 2 commits December 19, 2023 23:19
@ashfaq92 @fujitatomoya
Update source/Tutorials/Intermediate/Launch/Launch-system.rst
f6859c6 
Co-authored-by: Tomoya Fujita <[email protected]>
Signed-off-by: Muhammad Ashfaq <[email protected]>
@ashfaq92
Update Launch-system.rst
060fa33 
Signed-off-by: Muhammad Ashfaq <[email protected]>
clalancette
clalancette requested changes Dec 20, 2023
View reviewed changes
source/Tutorials/Intermediate/Launch/Launch-system.rst Outdated
Comment on lines 108 to 109
import os # ADD
from glob import glob # ADD
Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, I'm not a fan of this either. I agree that it is somewhat hard to understand what to change, but I think this is pretty ugly.

Instead, I'd like to think of another way to do this. Elsewhere where we have this pattern (like http://docs.ros.org/en/humble/Tutorials/Beginner-Client-Libraries/Using-Parameters-In-A-Class-Python.html#change-via-a-launch-file), we say something like:

"Add the import statements to the top of the file, and the other new statement to the data_files parameter to include all launch files.".

So I think we should do the same here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is looking better, but we should still remove the # ADD lines on these lines. Once that is done, I'm happy with this.

@ashfaq92
Update Launch-system.rst
0cac21e 
also copyedited the hard-to-read text. 

Signed-off-by: Muhammad Ashfaq <[email protected]>
@ashfaq92
Update Launch-system.rst
f0e8142 
Signed-off-by: Muhammad Ashfaq <[email protected]>
clalancette
clalancette requested changes Dec 20, 2023
View reviewed changes
source/Tutorials/Intermediate/Launch/Launch-system.rst Outdated
Comment on lines 108 to 109
import os # ADD
from glob import glob # ADD
Copy link
Contributor

Choose a reason for hiding this comment

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

This is looking better, but we should still remove the # ADD lines on these lines. Once that is done, I'm happy with this.

@ashfaq92
Update Launch-system.rst
4a6dce0 
Signed-off-by: Muhammad Ashfaq <[email protected]>
@ashfaq92
Update Launch-system.rst
60af0cd 
Signed-off-by: Muhammad Ashfaq <[email protected]>
@ashfaq92 ashfaq92 requested a review from clalancette December 20, 2023 14:57
@ashfaq92
Copy link
Contributor Author

it looks fine now

clalancette
clalancette approved these changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you for iterating.

@clalancette clalancette merged commit 482dcd2 into ros2:humble Dec 20, 2023
3 checks passed
@clalancette
Copy link
Contributor

@Mergifyio backport rolling iron

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 20, 2023 •
edited
Loading

backport rolling iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Dec 20, 2023
@ashfaq92 @mergify
Update Launch-system.rst (#4068)
2855326 
* Update Launch-system.rst

added comments to the code.
I think the it is better if we tell beginner what's changed, especially the addition of
```
import os
from glob import glob
```
Otherwise, a beginner might not notice this and will take some time to figure out what's not working (happened to me).

Other part of documentation followed this. For example:
https://docs.ros.org/en/humble/Tutorials/Intermediate/Tf2/Writing-A-Tf2-Broadcaster-Py.html#update-setup-py

Signed-off-by: Muhammad Ashfaq <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
(cherry picked from commit 482dcd2)

# Conflicts:
#	source/Tutorials/Intermediate/Launch/Launch-system.rst
@mergify mergify bot mentioned this pull request Dec 20, 2023
Merged
mergify bot pushed a commit that referenced this pull request Dec 20, 2023
@ashfaq92 @mergify
Update Launch-system.rst (#4068)
e0149dc 
* Update Launch-system.rst

added comments to the code.
I think the it is better if we tell beginner what's changed, especially the addition of
```
import os
from glob import glob
```
Otherwise, a beginner might not notice this and will take some time to figure out what's not working (happened to me).

Other part of documentation followed this. For example:
https://docs.ros.org/en/humble/Tutorials/Intermediate/Tf2/Writing-A-Tf2-Broadcaster-Py.html#update-setup-py

Signed-off-by: Muhammad Ashfaq <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
(cherry picked from commit 482dcd2)

# Conflicts:
#	source/Tutorials/Intermediate/Launch/Launch-system.rst
@mergify mergify bot mentioned this pull request Dec 20, 2023
Merged
clalancette pushed a commit that referenced this pull request Dec 20, 2023
@ashfaq92 @clalancette
Update Launch-system.rst (#4068)
8e7aedf 
* Update Launch-system.rst

added comments to the code.
I think the it is better if we tell beginner what's changed, especially the addition of
```
import os
from glob import glob
```
Otherwise, a beginner might not notice this and will take some time to figure out what's not working (happened to me).

Other part of documentation followed this. For example:
https://docs.ros.org/en/humble/Tutorials/Intermediate/Tf2/Writing-A-Tf2-Broadcaster-Py.html#update-setup-py

Signed-off-by: Muhammad Ashfaq <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
(cherry picked from commit 482dcd2)

# Conflicts:
#	source/Tutorials/Intermediate/Launch/Launch-system.rst
clalancette pushed a commit that referenced this pull request Dec 20, 2023
@ashfaq92 @clalancette
Update Launch-system.rst (#4068)
a3a7776 
* Update Launch-system.rst

added comments to the code.
I think the it is better if we tell beginner what's changed, especially the addition of
```
import os
from glob import glob
```
Otherwise, a beginner might not notice this and will take some time to figure out what's not working (happened to me).

Other part of documentation followed this. For example:
https://docs.ros.org/en/humble/Tutorials/Intermediate/Tf2/Writing-A-Tf2-Broadcaster-Py.html#update-setup-py

Signed-off-by: Muhammad Ashfaq <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
(cherry picked from commit 482dcd2)

# Conflicts:
#	source/Tutorials/Intermediate/Launch/Launch-system.rst
clalancette pushed a commit that referenced this pull request Dec 20, 2023
@mergify @ashfaq92
Update Launch-system.rst (#4068) (#4075)
20850ab 
* Update Launch-system.rst

added comments to the code.
I think the it is better if we tell beginner what's changed, especially the addition of
```
import os
from glob import glob
```
Otherwise, a beginner might not notice this and will take some time to figure out what's not working (happened to me).

Other part of documentation followed this. For example:
https://docs.ros.org/en/humble/Tutorials/Intermediate/Tf2/Writing-A-Tf2-Broadcaster-Py.html#update-setup-py

Signed-off-by: Muhammad Ashfaq <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
(cherry picked from commit 482dcd2)

# Conflicts:
#	source/Tutorials/Intermediate/Launch/Launch-system.rst

Co-authored-by: Muhammad Ashfaq <[email protected]>
clalancette pushed a commit that referenced this pull request Dec 20, 2023
@mergify @ashfaq92
Update Launch-system.rst (#4068) (#4076)
0056628 
* Update Launch-system.rst

added comments to the code.
I think the it is better if we tell beginner what's changed, especially the addition of
```
import os
from glob import glob
```
Otherwise, a beginner might not notice this and will take some time to figure out what's not working (happened to me).

Other part of documentation followed this. For example:
https://docs.ros.org/en/humble/Tutorials/Intermediate/Tf2/Writing-A-Tf2-Broadcaster-Py.html#update-setup-py

Signed-off-by: Muhammad Ashfaq <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
(cherry picked from commit 482dcd2)

# Conflicts:
#	source/Tutorials/Intermediate/Launch/Launch-system.rst

Co-authored-by: Muhammad Ashfaq <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya left review comments

@clalancette clalancette clalancette approved these changes

@audrow audrow Awaiting requested review from audrow audrow is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ashfaq92 @clalancette @fujitatomoya