-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add pre-commit hooks & BSD 3-Clause license #206
base: ros2-devel
Are you sure you want to change the base?
Conversation
Note to reviewers: the main files to review are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good, just want to make sure our READMEs are still readable after this PR gets merged.
@@ -1,5 +1,7 @@ | |||
#!/usr/bin/env python3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this and others, are there any instances where our existing stack calls Python files with ./
? My understanding is this would be the only reason to have the shebang line present, so I'm wondering if it was done intentionally. Anecdotally, I haven't seen any such cases, but it's good to double check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ROS uses it for running Python nodes, I think to allow developers to specify python2 vs python3.
- main | ||
- master | ||
- ros2-devel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the others besides ros2-devel
intended to catch the names of the production branches in other repositories, i.e. this pre-commit workflow will be applied to them as well if these pre-commit files are copied over?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I'll copy this directly into the other repos.
(I don't think additional branches should impact runtime, since this repo doesn't have branches by those names)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my intent was to use this as a template for the other ADA repositories. I believe it doesn't add any extra computation to add these extra branches to the config file, since the branches don't exist.
Description
This PR adds pre-commit hooks to this repository. This ensures consistent formatting and style throughout the repository. After the PR gets merged in, I'll be able to configure this repository to automatically run the pre-commit hooks on every PR.
Related PRs:
ada_ros2
#51feeding_web_interface
#150Testing procedure
Although in theory this should not change any functionality, we must check. Thus:
real
:python3 src/ada_Feeding/start.py
mock
:python3 src/ada_Feeding/start.py --sim mock
Before opening a pull request
pre-commit run --all-files
pylint --recursive=y --rcfile=.pylintrc .
. All warnings butfixme
must be addressed.Before Merging
Squash & Merge