Skip to content

typos corrected and support added for getting_started#587

Merged
mlautman merged 2 commits intomoveit:melodic-develfrom
Sunny478:melodic-devel
Feb 25, 2021
Merged

typos corrected and support added for getting_started#587
mlautman merged 2 commits intomoveit:melodic-develfrom
Sunny478:melodic-devel

Conversation

@Sunny478
Copy link
Copy Markdown

Due to some typos in _scripts/tutorialformatter.py, ./build_locally.sh was not running, corrected them.

If any user wanted to build 'moveit' from source using his/her forked repository (for later contribution), it gets a little confusing as the 'build from source' tutorial mentions cloning (with wstool) using the ros_planning/moveit repos only. Support added for how he/she can clone the forked repository following the same lines as the provided tutorial (wstool) by changing few lines in the .rosinstall files.

Description

Please explain the changes you made, including a reference to the related issue if applicable

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

…_started.rst, .build_locally.sh runs successfully now
@welcome
Copy link
Copy Markdown

welcome Bot commented Feb 23, 2021

Thanks for helping in improving MoveIt and open source robotics!

Copy link
Copy Markdown
Contributor

@felixvd felixvd 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 the PR. I think rather than explaining the whole procedure (which takes up screen space and won't apply to many readers), supplying the right approach and search terms for those who might get stuck will be better. I won't block merging if others think it's helpful enough to go in, though.

Comment thread doc/getting_started/getting_started.rst Outdated
sudo apt install ros-melodic-moveit

Advanced users might want to `install MoveIt from source <http://moveit.ros.org/install/source/>`_.
If you clone your own moveit fork into the workspace, disable the moveit repository in the .rosinstall file.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
If you clone your own moveit fork into the workspace, disable the moveit repository in the .rosinstall file.
If you want to build your own MoveIt fork, replace the repository uri in the .rosinstall file with your own.

I think just this sentence will allow people to find the solution if they encounter issues. Explaining the procedure in detail doesn't really fit with the rest of the tutorial, so I would just leave it out.

Comment thread _scripts/tutorialformatter.py Outdated
flattened_lines.extend( subs[sub_name] )
else:
print 'tutorialformatter.py error: sub-tutorial %s not found.' % sub_name
print ('tutorialformatter.py error: sub-tutorial %s not found.' % sub_name)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
print ('tutorialformatter.py error: sub-tutorial %s not found.' % sub_name)
print('tutorialformatter.py error: sub-tutorial " + sub_name + " not found.')

Removes the space before the bracket and the C-style string formatting (which belongs on a dump imo).

Also remove the space before the bracket in the other line please

Copy link
Copy Markdown
Contributor

@felixvd felixvd left a comment

Choose a reason for hiding this comment

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

LGTM

@mlautman mlautman merged commit 0fdea4d into moveit:melodic-devel Feb 25, 2021
@welcome
Copy link
Copy Markdown

welcome Bot commented Feb 25, 2021

Congrats on getting your first MoveIt pull request merged and improving open source robotics!

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