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

feat(dev): add hooks setup and pre-push hook with black formatter #101

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

guidodinello
Copy link
Contributor

@guidodinello guidodinello commented Dec 18, 2024

Add Git Hooks Support with Black Code Formatting

Overview

This PR adds support for Git hooks in ClaudeSync, starting with a pre-commit hook that automatically formats Python code using black before each commit.

Features

  • Uses Git native hooks system
  • Implements pre-commit hook for automatic code formatting with black
  • Provides easy hook installation via claudesync hooks install command
  • Modular design to support additional hooks in the future

Implementation Details

  • Added hooks.py module to handle hook installation and management
  • Added pre_commit.py template that formats staged Python files using black
  • Updated pyproject.toml to include black as optional dependency
  • Integrated hooks commands into ClaudeSync's CLI structure

Usage

After installing ClaudeSync, users can install the hooks with:

# Install with formatting dependencies
pip install -e .[format]

# Install hooks
claudesync hooks install

# List available hooks
claudesync hooks list

Comments

@jahwag I dont really know if this is a feature you want to incorporate to the project, I know some devs are not friends of this hooks workflow. Feel free to just decline this PR.

@jahwag
Copy link
Owner

jahwag commented Dec 18, 2024

This is a nice suggestion. Though introducing an alternate CLI does not quite align with the rest of the project. Additionally, registering "install-hooks" globally could potentially annoy some users. To avoid this, it might be better to integrate the "install-hooks" command within ClaudeSync's CLI under "hooks". This commit also need signing.

@guidodinello guidodinello force-pushed the feature/pre-push-hook branch 2 times, most recently from 4feb24c to bdb6d65 Compare December 20, 2024 02:29
Copy link
Owner

@jahwag jahwag left a comment

Choose a reason for hiding this comment

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

Upon further contemplation though this is nice to have for python developers i.e. someone working on claudesync. It’s difficult to see how it benefits the wider user base. It doesn’t feel like an entirely natural thing to have in claudesync. In java we would typically have a build tool plugin that would install the git hook, we wouldn’t distribute it with the software itself.

@@ -0,0 +1,77 @@
#!/usr/bin/env python3
Copy link
Owner

Choose a reason for hiding this comment

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

This approach didn’t work on Windows because only the python alias is mapped, leading to the error:
/usr/bin/env: 'python3': No such file or directory.

Since this issue could affect a number of users, it would be more robust to use #!/usr/bin/env python in the shebang for better cross-platform compatibility. To ensure the script runs with Python 3, you can add a runtime version check like this:

import sys
if sys.version_info.major < 3:
    print("Error: This script requires Python 3. Please install and configure it.")
    sys.exit(1)

This ensures that the script will fail gracefully if the system default python points to Python 2. It also avoids requiring users to set up the python3 alias, which may not be available by default on Windows.

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.

2 participants