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

Point users to space_robots demo at the end of moveit2 README (#85) #84

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

sea-bass
Copy link
Contributor

@sea-bass sea-bass commented Oct 31, 2023

I was doing some testing with these Docker images to give an official MoveIt maintainer sign-off for working MoveIt2 images.

I was able to run both the MoveIt2 Tutorials RViz example as per the moveit2 image, and also the Canadarm demo in the space_robots image.

Just submitting a few small tweaks based on my trying to follow the instructions, but otherwise 👍🏻

Closes #85


Quick question -- can I also remove all the $ from all the README Instructions, like

$ ./run.sh

because it makes it annoying to copy paste -- I have to manually select the text or remove the first few characters instead of copying the entire line.

@ivanperez-keera
Copy link
Contributor

@sea-bass We are now following a process described in https://github.com/space-ros/space-ros/blob/main/CONTRIBUTING.md.

Could I trouble you to please open an issue detailing what's missing/wrong about the current instructions, so that we can associate this PR to it (including mentioning the issue by number in the summary line of the commit itself) before we merge it? Thanks!

@sea-bass
Copy link
Contributor Author

@ivanperez-keera Done -- linked to an issue. Do you mind answering my open question in the PR description in case we want to also tackle that in this PR vs. take no action? Thanks!

@Bckempa Bckempa added this to the humble-2024.01.0 milestone Dec 3, 2023
@sea-bass sea-bass closed this Dec 14, 2023
@sea-bass sea-bass reopened this Dec 14, 2023
@EzraBrooks
Copy link
Member

I agree with removing the $ from the instruction blocks - while you are removing them, could you also indicate to markdown that they are blocks of bash?

@ivanperez-keera
Copy link
Contributor

Could you also adjust the commit message to point to the issue the way we are doing this in other commits? (Sorry, I saw your comment but we are relying on commits to point to issues, not github's method of linking things.)

@ivanperez-keera
Copy link
Contributor

This issue addresses multiple concerns (typos, like breaking, the $ thing). I know they are all little, but my preference would be to break it up if they are unrelated (and yes, I know it's annoying :( )

@Bckempa
Copy link
Contributor

Bckempa commented Dec 18, 2023

@ivanperez-keera since @sea-bass isn't really working on Space ROS and was just doing us a favor as part of the first release you might need to be the change you want to see in the PR on this one.

Also can you clarify if you want the different types of changes you mentioned broken up to different issues, PRs, commits, or some combination?

@sea-bass
Copy link
Contributor Author

sea-bass commented Dec 18, 2023

@ivanperez-keera since @sea-bass isn't really working on Space ROS and was just doing us a favor as part of the first release you might need to be the change you want to see in the PR on this one.

Also can you clarify if you want the different types of changes you mentioned broken up to different issues, PRs, commits, or some combination?

I'm happy to split these up this week, but would agree that having the list of which parts to break down would be useful. I'm seeing:

  1. Adding the extra section on how to go from building the Docker image to running MoveIt examples
  2. Fixing typos
  3. Applying the one sentence per line principle
  4. Removing the $ from documented commands for easier copy-pastability and adding bash formatting

Does this sound good?

@ivanperez-keera
Copy link
Contributor

ivanperez-keera commented Jan 11, 2024

Hi @sea-bass. Sorry that it took me a bit longer.

Thanks a lot for your understanding of our process and for showing flexibility 😃 . We're trying our best to be thorough.

I've split this into several issues:

EDIT: Changed because I went ahead and filed PRs for the smaller things.

@sea-bass Can you please modify this this PR (number #84), to retain only the changes pertaining to #85 specifically (the relation between the Moveit2 and the Robots demos), without any style, formatting or typo changes.

@ivanperez-keera ivanperez-keera changed the title Small tweaks to READMEs Point users to space_robots demo at the end of moveit2 README Jan 11, 2024
@ivanperez-keera ivanperez-keera force-pushed the small-instructions-fixes branch from 92f4eb3 to 4230bb8 Compare January 11, 2024 12:45
@sea-bass sea-bass force-pushed the small-instructions-fixes branch from 4230bb8 to 30f3636 Compare January 11, 2024 14:13
@sea-bass
Copy link
Contributor Author

@ivanperez-keera thank you for doing a lot of this PR surgery on my behalf -- much appreciated. I've updated this PR to only address #85.

@ivanperez-keera
Copy link
Contributor

Thank you, @sea-bass . Merging.

@ivanperez-keera ivanperez-keera changed the title Point users to space_robots demo at the end of moveit2 README Point users to space_robots demo at the end of moveit2 README (#85) Jan 11, 2024
@ivanperez-keera ivanperez-keera merged commit 3e87f73 into space-ros:main Jan 11, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Clarify installation instructions for MoveIt2 and Space Robots Demo
4 participants