Skip to content

Conversation

@leander-dsouza
Copy link
Contributor

Description

I have added the following hooks as part of the pre-commit:

  • end-of-file-fixer
  • mixed-line-ending
  • trailing-whitespace

This prevents empty whitespace and additional newlines from registering as a separate commit during development.

Additional Information

  • We can integrate codespell and sphinx-lint into the pre-commit, hence simplifying the workflow.

@leander-dsouza leander-dsouza marked this pull request as ready for review March 31, 2025 15:26
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@leander-dsouza thanks for the PR.

IMO this is nice to have to have automated github action for pre-commit tool, that actually fixes some styles with this PR. I think we could aim small and miss small to enable this pre-commit github action for some repositories as 1st step. and then, we could have centralized github action for pre-commit and used by each repository.

before further development, i would like to bring this topic to ROS Project Management Committee meeting to get more feedback from other committers and maintainers. please let me have some time to get back to you on this.

btw, i think title of this PR is kinda misleading? precisely it could be more like Add pre-commit github action, not git pre-commit hook? at least, this PR does not push that to the developer's local environment.

@leander-dsouza
Copy link
Contributor Author

Thank you for the feedback, @fujitatomoya.
I will eagerly wait for your response regarding the nature of the pre-commit addition.

@leander-dsouza leander-dsouza changed the title Added base pre-commit hooks to include whitespace and eol checks. Add pre-commit github action Apr 1, 2025
@christophebedard
Copy link
Member

Not sure about other repos (e.g., ros2/rclcpp), but I think this makes sense for this repo.

@fujitatomoya
Copy link
Collaborator

@leander-dsouza

we had discussion on this in last PMC meeting, and it turned out that there are some security concerns to use pre-commit framework.
running pre-commit locally exposes users to potential risks from untrusted code in external hooks, pre-commit doesn’t require explicit approval for changes, making it easier for malicious modifications to go unnoticed.
that said, indirect dependencies introduce risks of being altered without direct control, and this is actually easy to happen for user once they install pre-commit in local environment.

no strong consensus for now, but general agreement to explore alternative implementations (e.g., shell scripts, Python integrations).

i thought that probably we can only use official pre-commit-hooks only, that is what this PR does, besides we can tag the specific commit hash.
but this does not really address the security concern described above...

at this moment, we can remove pre-commit things from this PR, and some cosmetic fixes can be merged.

Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>
@leander-dsouza leander-dsouza changed the title Add pre-commit github action Remove trailing-whitespace Apr 3, 2025
@leander-dsouza
Copy link
Contributor Author

Thank you for the explanation @fujitatomoya :)
I have refactored the PR only to remove the trailing whitespace in the codebase.

@fujitatomoya fujitatomoya merged commit 6e5a588 into ros2:rolling Apr 3, 2025
5 checks passed
mergify bot pushed a commit that referenced this pull request Apr 3, 2025
Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>
(cherry picked from commit 6e5a588)

# Conflicts:
#	source/Tutorials/Intermediate/Publishing-Messages-Using-YAML-Files.rst
mergify bot pushed a commit that referenced this pull request Apr 3, 2025
Signed-off-by: Leander Stephen D'Souza <leanderdsouza1234@gmail.com>
(cherry picked from commit 6e5a588)

# Conflicts:
#	source/Tutorials/Intermediate/Publishing-Messages-Using-YAML-Files.rst
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