Conversation
Add comprehensive Claude skills for development assistance: - git-workflow: Git operations and best practices - shell-scripting: Bash/Zsh scripting and automation - dotfiles-manager: Dotfiles organization and management - debug-helper: Debugging and troubleshooting strategies - performance-optimizer: Code and script optimization - test-expert: Testing and TDD practices Each skill provides specialized domain expertise that Claude can invoke for targeted assistance. Also updated README to document the skills and agents available in this repository.
Summary of ChangesHello @einverne, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the AI assistance capabilities by integrating a suite of new Claude Code skills. These skills provide targeted expertise across critical development domains, aiming to streamline workflows and improve code quality. The changes also ensure that these new capabilities are properly documented for users. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of Claude Code skills, which is a great enhancement for AI-assisted development within this repository. The skills cover essential areas like git, shell scripting, debugging, performance, and testing. The documentation is well-written and detailed. I've identified a few issues in the provided code examples and documentation links that should be addressed to improve their quality and correctness. My review includes suggestions to fix a broken link, correct buggy code in a shell script template, and improve the clarity of several code examples.
| while [[ $# -gt 0 ]]; do | ||
| case $1 in | ||
| -h|--help) | ||
| usage | ||
| exit 0 | ||
| ;; | ||
| -v|--verbose) | ||
| VERBOSE=true | ||
| shift | ||
| ;; | ||
| *) | ||
| echo "Unknown option: $1" | ||
| usage | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| done |
There was a problem hiding this comment.
The argument parsing logic in the "Safe Script Template" is flawed. It will incorrectly handle positional arguments by treating them as unknown options and exiting. For a script that accepts both options and arguments (like script.sh -v mycommand), this loop will fail. A more robust approach is needed to distinguish between options and positional arguments. The suggested change will stop parsing options upon encountering the first non-option argument.
| while [[ $# -gt 0 ]]; do | |
| case $1 in | |
| -h|--help) | |
| usage | |
| exit 0 | |
| ;; | |
| -v|--verbose) | |
| VERBOSE=true | |
| shift | |
| ;; | |
| *) | |
| echo "Unknown option: $1" | |
| usage | |
| exit 1 | |
| ;; | |
| esac | |
| done | |
| while [[ $# -gt 0 ]]; do | |
| case $1 in | |
| -h|--help) | |
| usage | |
| exit 0 | |
| ;; | |
| -v|--verbose) | |
| VERBOSE=true | |
| shift | |
| ;; | |
| -*) | |
| echo "Unknown option: $1" | |
| usage | |
| exit 1 | |
| ;; | |
| *) | |
| break | |
| ;; | |
| esac | |
| done |
| trap cleanup EXIT | ||
| trap 'echo "Error on line $LINENO"' ERR | ||
|
|
||
| cleanup() { | ||
| # Cleanup code | ||
| rm -f "$TEMP_FILE" | ||
| } | ||
| ``` |
There was a problem hiding this comment.
In the "Error Handling" example, the cleanup function attempts to remove a file using the variable $TEMP_FILE, but this variable is never defined in the script template. This makes the example incomplete and incorrect. The example should show how to create a temporary file securely, for instance using mktemp.
| trap cleanup EXIT | |
| trap 'echo "Error on line $LINENO"' ERR | |
| cleanup() { | |
| # Cleanup code | |
| rm -f "$TEMP_FILE" | |
| } | |
| ``` | |
| TEMP_FILE=$(mktemp) | |
| trap cleanup EXIT | |
| trap 'echo "Error on line $LINENO"' ERR | |
| cleanup() { | |
| # Cleanup code | |
| rm -f "$TEMP_FILE" | |
| } |
|
|
||
| # Assert | ||
| assert user.email == email | ||
| assert user.is_active == True |
There was a problem hiding this comment.
In Python, comparing a boolean value with == True is redundant and not idiomatic. It's better to assert the truthiness of the value directly.
| assert user.is_active == True | |
| assert user.is_active |
| @patch('myapp.external_api_call') | ||
| def test_function(mock_api): | ||
| mock_api.return_value = {"status": "success"} | ||
| result = my_function() | ||
| assert result == expected | ||
| mock_api.assert_called_once_with(expected_arg) | ||
| ``` |
There was a problem hiding this comment.
This Python mocking example is incomplete because the variables expected and expected_arg are used without being defined. This can be confusing for someone trying to understand or use the example. Please define these variables or use concrete values to make the example self-contained and runnable.
| @patch('myapp.external_api_call') | |
| def test_function(mock_api): | |
| mock_api.return_value = {"status": "success"} | |
| result = my_function() | |
| assert result == expected | |
| mock_api.assert_called_once_with(expected_arg) | |
| ``` | |
| @patch('myapp.external_api_call') | |
| def test_function(mock_api): | |
| expected_return_value = {"status": "success"} | |
| mock_api.return_value = expected_return_value | |
| result = my_function() | |
| assert result == expected_return_value | |
| mock_api.assert_called_once_with("some_expected_argument") |
| @pytest.mark.asyncio | ||
| async def test_async_function(): | ||
| result = await async_function() | ||
| assert result == expected |
There was a problem hiding this comment.
The expected variable is not defined in this async test example, which makes it incomplete. It should be replaced with an actual expected value for clarity.
| assert result == expected | |
| assert result == "expected_async_result" |
| def test_time_dependent(mock_datetime): | ||
| mock_datetime.now.return_value = datetime(2024, 1, 1) | ||
| result = function_using_time() | ||
| assert result == expected |
There was a problem hiding this comment.
The expected variable is not defined in this time-dependent test example, making it incomplete. Please replace it with a concrete expected value that would result from the mocked time.
| assert result == expected | |
| assert result == "expected_value_for_2024-01-01" |
- Reorganize skills from flat .md files to directory structure with SKILL.md files following official Claude Code format - Update all skill descriptions to include "when to use" guidance following progressive disclosure pattern - Add skills symlink in bootstrap.conf.yml to automatically link ~/.claude/skills to dotfiles/.claude/skills during setup - Update skills README with: * Official frontmatter requirements (name, description limits) * Directory structure documentation * Progressive disclosure explanation * Installation instructions via dotbot * Links to official documentation Each skill now follows the official structure: - Directory per skill (e.g., git-workflow/) - SKILL.md file with YAML frontmatter - name: lowercase-with-hyphens (max 64 chars) - description: what it does + when to use it (max 1024 chars) The bootstrap process now creates ~/.claude/ and symlinks skills automatically, making them available to Claude Code immediately.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of Claude Code skills for various development tasks, along with documentation and integration into the dotfiles bootstrap process. The new skills are well-documented and cover important areas like Git, shell scripting, debugging, and performance. My review includes suggestions to fix a broken link, improve command examples and code templates for robustness and clarity, and update a deprecated GitHub Action to ensure CI stability. These changes will enhance the quality and usability of the new skills.
| while [[ $# -gt 0 ]]; do | ||
| case $1 in | ||
| -h|--help) | ||
| usage | ||
| exit 0 | ||
| ;; | ||
| -v|--verbose) | ||
| VERBOSE=true | ||
| shift | ||
| ;; | ||
| *) | ||
| echo "Unknown option: $1" | ||
| usage | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| done |
There was a problem hiding this comment.
The argument parsing loop in the "Safe Script Template" does not correctly handle positional arguments. Any positional argument will be treated as an "Unknown option," causing the script to exit prematurely. A robust argument parsing loop should distinguish between options and positional arguments. The suggested implementation stops parsing options upon encountering the first non-option argument, which is a common and effective pattern.
| while [[ $# -gt 0 ]]; do | |
| case $1 in | |
| -h|--help) | |
| usage | |
| exit 0 | |
| ;; | |
| -v|--verbose) | |
| VERBOSE=true | |
| shift | |
| ;; | |
| *) | |
| echo "Unknown option: $1" | |
| usage | |
| exit 1 | |
| ;; | |
| esac | |
| done | |
| while [[ $# -gt 0 ]]; do | |
| case $1 in | |
| -h|--help) | |
| usage | |
| exit 0 | |
| ;; | |
| -v|--verbose) | |
| VERBOSE=true | |
| shift | |
| ;; | |
| -*) | |
| echo "Unknown option: $1" >&2 | |
| usage | |
| exit 1 | |
| ;; | |
| *) | |
| break # stop parsing options | |
| ;; | |
| esac | |
| done |
| pip install -r requirements-dev.txt | ||
| pytest --cov --cov-report=xml | ||
| - name: Upload coverage | ||
| uses: codecov/codecov-action@v2 |
There was a problem hiding this comment.
The codecov/codecov-action@v2 GitHub Action is deprecated and may stop working in the future. You should update it to a more recent version, such as v4, to ensure continued support, security updates, and access to new features.
| uses: codecov/codecov-action@v2 | |
| uses: codecov/codecov-action@v4 |
| - [Claude Code Skills Documentation](https://docs.claude.com/en/docs/claude-code/skills) | ||
| - [Official Skills Repository](https://github.com/anthropics/skills) | ||
| - [How to Create Custom Skills](https://support.claude.com/en/articles/12512198-how-to-create-custom-skills) | ||
| - [Agents Directory](../agents/README.md) - For autonomous task execution |
There was a problem hiding this comment.
The relative link to the Agents Directory README appears to be incorrect. Given the file structure, the path should likely be ../../claude/agents/README.md to correctly navigate from .claude/skills/ up two levels and then into claude/agents/.
| - [Agents Directory](../agents/README.md) - For autonomous task execution | |
| - [Agents Directory](../../claude/agents/README.md) - For autonomous task execution |
| ### Check if Service is Running | ||
| ```bash | ||
| # Check process | ||
| ps aux | grep service_name |
There was a problem hiding this comment.
The ps aux | grep service_name command will include the grep process itself in the output. A common technique to prevent this is to use a character class for the first letter of the search term, which prevents grep from matching its own process line.
| ps aux | grep service_name | |
| ps aux | grep [s]ervice_name |
| brew install stow | ||
| elif [[ "$(uname)" == "Linux" ]]; then | ||
| # Linux | ||
| sudo apt-get install stow |
There was a problem hiding this comment.
For Debian-based systems, it's a good practice to update the package list before installing a new package to ensure you're getting the latest version available. Also, using the -y flag will automatically confirm the installation, which is useful in non-interactive scripts.
| sudo apt-get install stow | |
| sudo apt-get update && sudo apt-get install -y stow |
|
|
||
| 2. **Commit Best Practices** | ||
| - Write clear, conventional commit messages | ||
| - Follow format: `type(scope): description` |
There was a problem hiding this comment.
The commit message format type(scope): description follows the Conventional Commits specification. It would be helpful for users if you included a link to the official specification for more details.
| - Follow format: `type(scope): description` | |
| - Follow format: `type(scope): description` (see [Conventional Commits](https://www.conventionalcommits.org/)) |
| ``` | ||
|
|
||
| ### Find Memory Leaks | ||
| ```bash |
There was a problem hiding this comment.
The code block for finding memory leaks is incorrectly marked as bash. The content, which includes the @profile decorator and sys.getrefcount, is Python code. The language specifier for the code block should be python for correct syntax highlighting and clarity.
| ```bash | |
| ```python |
| trap cleanup EXIT | ||
| trap 'echo "Error on line $LINENO"' ERR | ||
|
|
||
| cleanup() { | ||
| # Cleanup code | ||
| rm -f "$TEMP_FILE" | ||
| } |
There was a problem hiding this comment.
The cleanup function example refers to $TEMP_FILE, but this variable is not defined within the snippet, which could be confusing. To make the example clearer and promote safe scripting practices, it's beneficial to show how such a temporary file should be created using mktemp and to check if the command was successful.
| trap cleanup EXIT | |
| trap 'echo "Error on line $LINENO"' ERR | |
| cleanup() { | |
| # Cleanup code | |
| rm -f "$TEMP_FILE" | |
| } | |
| TEMP_FILE=$(mktemp) || exit 1 | |
| trap cleanup EXIT | |
| trap 'echo "Error on line $LINENO"' ERR | |
| cleanup() { | |
| # Cleanup code | |
| rm -f "$TEMP_FILE" | |
| } |
|
|
||
| # Assert | ||
| assert user.email == email | ||
| assert user.is_active == True |
There was a problem hiding this comment.
In Python, comparing a boolean value with == True is redundant and not considered idiomatic. It's more Pythonic to assert the truthiness of the value directly.
| assert user.is_active == True | |
| assert user.is_active |
einverne
deleted the
claude/add-claude-skills-support-011CUp3XhfBzhC3jV3N6z1AZ
branch
2 participants
Add comprehensive Claude skills for development assistance:
Each skill provides specialized domain expertise that Claude can invoke for targeted assistance. Also updated README to document the skills and agents available in this repository.