Skip to content

Add pre-commit hooks with pyink and mypy#91

Merged
alxmrs merged 3 commits into
xqlsystems:mainfrom
jayendra-info:setup-pre-commit-mypy
Dec 3, 2025
Merged

Add pre-commit hooks with pyink and mypy#91
alxmrs merged 3 commits into
xqlsystems:mainfrom
jayendra-info:setup-pre-commit-mypy

Conversation

@jayendra-info

Copy link
Copy Markdown
Contributor

This commit sets up automated code quality checks using pre-commit:

  1. Pre-commit configuration (.pre-commit-config.yaml):

    • Added pyink for code formatting (reads config from pyproject.toml)
    • Added mypy for type checking
    • Added standard pre-commit hooks (trailing whitespace, end-of-file, etc.)
  2. Updated pyproject.toml:

    • Added mypy and pre-commit to dev dependencies
    • Added pandas-stubs for better type checking
    • Configured mypy with per-module ignores for libraries without stubs
    • Set python_version to 3.11 to match .python-version file
  3. Fixed type errors in xarray_sql/df.py:

    • Fixed tuple concatenation type issues in block_slices()
    • Fixed Callable type annotation syntax in from_map_batched()
    • Added type ignore for xarray return type in pivot()
  4. Updated CONTRIBUTING.md:

    • Documented pre-commit setup steps
    • Updated formatting/type-checking instructions
  5. Updated GitHub Actions lint workflow:

    • Added mypy type checking step
    • Changed to use uv run for consistency
  6. Auto-fixed formatting issues:

    • Removed trailing whitespace
    • Fixed end-of-file newlines
    • Applied pyink formatting

All hooks pass successfully and type checking works correctly.

This commit sets up automated code quality checks using pre-commit:

1. Pre-commit configuration (.pre-commit-config.yaml):
   - Added pyink for code formatting (reads config from pyproject.toml)
   - Added mypy for type checking
   - Added standard pre-commit hooks (trailing whitespace, end-of-file, etc.)

2. Updated pyproject.toml:
   - Added mypy and pre-commit to dev dependencies
   - Added pandas-stubs for better type checking
   - Configured mypy with per-module ignores for libraries without stubs
   - Set python_version to 3.11 to match .python-version file

3. Fixed type errors in xarray_sql/df.py:
   - Fixed tuple concatenation type issues in block_slices()
   - Fixed Callable type annotation syntax in from_map_batched()
   - Added type ignore for xarray return type in pivot()

4. Updated CONTRIBUTING.md:
   - Documented pre-commit setup steps
   - Updated formatting/type-checking instructions

5. Updated GitHub Actions lint workflow:
   - Added mypy type checking step
   - Changed to use uv run for consistency

6. Auto-fixed formatting issues:
   - Removed trailing whitespace
   - Fixed end-of-file newlines
   - Applied pyink formatting

All hooks pass successfully and type checking works correctly.

@alxmrs alxmrs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks great so far! Thank you for the PR. :)

Comment thread .github/workflows/lint.yml Outdated

@alxmrs alxmrs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One optional suggestion. Thanks so much for this change!

Comment thread .github/workflows/lint.yml Outdated
- name: Install xarray_sql
run: uv sync --dev
- name: Run pre-commit
run: uv run pre-commit run --all-files

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Super nit pick: I think you could skip installing the project (the step above) if you use uvx instead of uv run here. The benefit is a slightly faster lint check.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Counter-point to this suggestion: I may be forgetting that mypy needs to have the project installed + local deps. Would you mind testing this?

This is totally optional, I am happy with the change as is, too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks pointing it out, my python (and it's ecosystem knowledge ) is getting bit rusty, due to current focus on TS :) . Handled in the latest commit, please check, tested it in the isolated docker environment to make sure it works in the github actions as well, without any cached environment.

Comment thread .pre-commit-config.yaml
rev: 24.10.1
hooks:
- id: pyink
# Configuration is read from pyproject.toml [tool.pyink]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for this comment.

@alxmrs alxmrs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, thanks for the patch and the revisions.

@alxmrs
alxmrs marked this pull request as ready for review December 3, 2025 00:48
@alxmrs
alxmrs merged commit fa436e8 into xqlsystems:main Dec 3, 2025
7 checks passed
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