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

feat: ros include guard #32

Merged
merged 25 commits into from
Feb 24, 2022
Merged

feat: ros include guard #32

merged 25 commits into from
Feb 24, 2022

Conversation

isamu-takagi
Copy link
Collaborator

Check include guard.

@isamu-takagi isamu-takagi self-assigned this Feb 19, 2022
@isamu-takagi isamu-takagi force-pushed the ros-include-guard branch 2 times, most recently from fb461cc to 40ecb4e Compare February 19, 2022 09:34
@isamu-takagi isamu-takagi changed the title Add ros include guard feat: ros include guard Feb 19, 2022
@isamu-takagi
feat: ros include guard
90c3c61 
Signed-off-by: Takagi, Isamu <isamu.takagi@tier4.jp>
kenji-miyake
kenji-miyake reviewed Feb 19, 2022
View reviewed changes
Copy link
Contributor

@kenji-miyake kenji-miyake left a comment

Choose a reason for hiding this comment

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

Thank you! Could you show me your test results?
Or if possible, adding tests is better.

.pre-commit-hooks.yaml Outdated Show resolved Hide resolved
@isamu-takagi
feat: tests
c475144 
Signed-off-by: Takagi, Isamu <isamu.takagi@tier4.jp>
@isamu-takagi
feat: CI for tests
f5f2dc8
@isamu-takagi
Fix: CI
f24bee4
@isamu-takagi
fix: CI
19f10b6
@isamu-takagi
Copy link
Collaborator Author

Added a test. It can be checked with the pytest command at the repository root.

@kenji-miyake
Copy link
Contributor

kenji-miyake commented Feb 20, 2022 •
edited
Loading

Great, thank you!
And is it possible to add a workflow of GitHub Actions as well? -> Oh, you already did it, sorry.

kenji-miyake
kenji-miyake reviewed Feb 20, 2022
View reviewed changes
Copy link
Contributor

@kenji-miyake kenji-miyake left a comment

Choose a reason for hiding this comment

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

Thank you for you work!
Added some style-related comments.

README.md Outdated Show resolved Hide resolved
pre_commit_hooks/ros_include_guard.py Outdated Show resolved Hide resolved
pre_commit_hooks/ros_include_guard.py Outdated Show resolved Hide resolved
pre_commit_hooks/ros_include_guard.py Outdated Show resolved Hide resolved
pre_commit_hooks/ros_include_guard.py Outdated Show resolved Hide resolved
tests/resources/rospkg3/package.xml Outdated Show resolved Hide resolved
tests/resources/rospkg2/package.xml Outdated Show resolved Hide resolved
tests/test_ros_include_guard.py Outdated Show resolved Hide resolved
tests/test_ros_include_guard.py Outdated Show resolved Hide resolved
pre_commit_hooks/ros_include_guard.py Outdated Show resolved Hide resolved
@kenji-miyake kenji-miyake mentioned this pull request Feb 22, 2022
Merged
@kenji-miyake kenji-miyake mentioned this pull request Feb 23, 2022
Merged
@isamu-takagi isamu-takagi enabled auto-merge (squash) February 24, 2022 05:57
@kenji-miyake
Copy link
Contributor

@isamu-takagi Could you create a draft PR to autoware.universe or some other places and test this hook?

@isamu-takagi
Copy link
Collaborator Author

I created it here.

@isamu-takagi isamu-takagi mentioned this pull request Feb 24, 2022
Merged
7 tasks
kenji-miyake
kenji-miyake reviewed Feb 24, 2022
View reviewed changes
Copy link
Contributor

@kenji-miyake kenji-miyake left a comment

Choose a reason for hiding this comment

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

Generally LGTM, but added some comments.

pre_commit_hooks/ros_include_guard.py Show resolved Hide resolved
pre_commit_hooks/ros_include_guard.py Outdated Show resolved Hide resolved
pre_commit_hooks/ros_include_guard.py Outdated Show resolved Hide resolved
pre_commit_hooks/ros_include_guard.py Show resolved Hide resolved
pre_commit_hooks/ros_include_guard.py Show resolved Hide resolved
pre_commit_hooks/ros_include_guard.py Outdated Show resolved Hide resolved
pre_commit_hooks/ros_include_guard.py Outdated Show resolved Hide resolved
pre_commit_hooks/ros_include_guard.py Outdated Show resolved Hide resolved
kenji-miyake
kenji-miyake approved these changes Feb 24, 2022
View reviewed changes
Copy link
Contributor

@kenji-miyake kenji-miyake left a comment

Choose a reason for hiding this comment

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

Great work! Thank you!

@isamu-takagi isamu-takagi merged commit 0f0ed0f into main Feb 24, 2022
@isamu-takagi isamu-takagi deleted the ros-include-guard branch February 24, 2022 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenji-miyake kenji-miyake kenji-miyake approved these changes

@KeisukeShima KeisukeShima Awaiting requested review from KeisukeShima

Assignees

@isamu-takagi isamu-takagi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@isamu-takagi @kenji-miyake @KeisukeShima