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

Set pre commit as dev dependency #166

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

burningion
Copy link

This is a minor change, but adds pre-commit as a dev dependency, and clarifies instructions in the README about how to use it, all within uv. (When I went to run pre-commit, it wasn't already on my PATH.)

@joaomdmoura
Copy link
Collaborator

Disclaimer: This review was made by a crew of AI Agents.

Code Review Comment for PR #166

Overview

This pull request effectively implements improvements to how we manage pre-commit as a development dependency and updates various package versions in the uv.lock file. Below are my detailed observations and suggestions:

1. Changes to README.md

Change:

-pre-commit install
+uv run pre-commit install

Comments:

  • This update enhances consistency in the setup instructions, ensuring developers use the uv package manager, which aligns with the project's intended dependency management strategy.
  • Potential Impact: This helps prevent installation errors and misconfigurations, particularly for new contributors familiarizing themselves with the project’s setup.

2. Changes to pyproject.toml

Addition:

[dependency-groups]
dev = [
    "pre-commit>=4.0.1",
]

Comments:

  • Categorizing pre-commit under development dependencies is a best practice. This change separates development tools from production dependencies, enhancing clarity.
  • Suggestion for Improvement: Consider expanding this section to include other development-only dependencies for better organization, such as:
[dependency-groups]
dev = [
    "pre-commit>=4.0.1",
    "pytest>=8.0.0", 
    "pyright>=1.1.350",
    # Additional dev tools as necessary...
]

3. Changes to uv.lock

Key Updates:

  • Updated platform markers to:
-"python_full_version < '3.11' and platform_system != 'Windows'"
+"python_full_version < '3.11' and sys_platform != 'win32'"
  • Bumped versions for critical packages like aiohttp, pydantic, and added scrapegraph-py==1.8.0.

Comments:

  • Using sys_platform over platform_system is encouraged, presenting a more standardized approach.
  • Minor version updates generally imply enhancements and bug fixes across packages, which is beneficial for overall project stability.
  • Documentation Suggestion: To enhance transparency regarding these updates, it would be helpful to maintain a changelog or release notes indicating:
## Package Updates
- `aiohttp` upgraded to `3.11.11` for improved async support.
- `pydantic` upgraded to `2.10.4` for better type validation.
- Added `scrapegraph-py 1.8.0` to facilitate web scraping capabilities.

Additional Recommendations:

  • Consider introducing a .pre-commit-config.yaml file if not already present to define hooks that enforce coding standards and checks:
repos:
-   repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.0.1
    hooks:
    -   id: trailing-whitespace
    -   id: end-of-file-fixer
    -   id: check-yaml
    -   id: check-added-large-files

Summary

The adjustments made through this pull request significantly enhance the management of development dependencies, keep the project aligned with modern Python practices, and assure compatibility with contemporary library versions. Consider implementing the suggested additions for improved maintenance and clarity.

Overall, this PR is well-structured and ready for merging, with changes that positively impact the developer experience and project reliability.

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