fix: fixes pre-commit-config and adds type-checking; updates poe tasks #547
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues in the pre-commit configuration and updates the poe tasks in pyproject.toml to improve type-checking and overall clarity.
- Updated poe task definitions with additional help text and renamed tasks for clarity.
- Adjusted pre-commit hook configurations for Ruff, Prettier, Poetry, and MyPy.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pyproject.toml | Updated poe tasks with added help messages and task renaming, and removed isort skip exclusion. |
| .pre-commit-config.yaml | Adjusted exclusion regex formatting, updated the Prettier hook, and added new hooks for Poetry check and MyPy. |
Comments suppressed due to low confidence (1)
.pre-commit-config.yaml:48
- Ensure that replacing the 'types_or' option with the explicit '--write' argument in the Prettier hook is intentional and that it does not affect file type detection required for your formatting checks.
args: [--write]
pnilan
added
airbyte-python-cdk
bug
📝 WalkthroughWalkthroughThe updates modify the Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Pre-commit
participant Poetry
participant Mypy
Developer->>Pre-commit: Run pre-commit hooks
Pre-commit->>Poetry: Run poetry check (lockfile validation)
Pre-commit->>Mypy: Run mypy type checks on airbyte_cdk/
Pre-commit-->>Developer: Report results
Would you like me to dive deeper into the new pre-commit hook interactions or keep it at this high-level overview? Wdyt? Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
.pre-commit-config.yaml (2)
48-48: Explicit--writefor Prettier. Replacingtypes_orwithargs: [--write]aligns nicely with the corresponding Poe task; should we also scope this hook with afiles:pattern (e.g.,\.(js|ts|css|md)$) to limit it to relevant file types? wdyt?
60-67: Addpoetry-checkhook for lockfile validation. Great to enforcepoetry checkpre-commit—aligns with_check-lockfile. Might it help to restrict this hook to only run whenpyproject.tomlorpoetry.lockchange (via afiles:regex)? wdyt?pyproject.toml (7)
131-131: Consider clarifyinginstallhelp text. Theinstalltask runspoetry install --all-extras; should we update itshelpto explicitly mention "all extras"? wdyt?
140-140: Updatedformat-checksequence. Using_format-check-ruffand_format-check-prettieris clear; would adding--quietor--diffflags help surface only the relevant issues? wdyt?
154-154:lint-fixhelp clarity. The description mentions "excluding 'unsafe' fixes"; would specifying a brief example or linking to Ruff docs improve usability? wdyt?
155-155: Documentlint-fix-unsafeworkflow. The help hints at best practice; perhaps we could reference an example or note that manual review is required for these fixes? wdyt?
164-164: Adjustcheck-allsequence. Including_check-lockfileat the end is logical; should we reorder to havetype-checkrun beforeformat-checkso type errors surface earlier? wdyt?
170-170:pytest-fastfilter redundancy. You’re using both--skipslowand-m 'not slow'; could we simplify by choosing one mechanism for excluding slow tests? wdyt?
177-177: CI task sequence clarity. Omittingtype-checkfromcheck-cimakes sense for faster CI, but should we note that detail in the help text to avoid confusion? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
.pre-commit-config.yaml(4 hunks)pyproject.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (7)
.pre-commit-config.yaml (2)
13-13: Exclude.ruff_cacheadded for consistency. This ensures Ruff’s cache directory is ignored alongside other tool caches.
68-76: Introduce mypy pre-commit hook. Nice to add type checking forairbyte_cdk/. Can we verify thatmypy.inilives at the repo root so this hook picks up the correct config? wdyt?pyproject.toml (5)
132-132: Publiclocktask exposure. Thelocktask runspoetry lock—do we want to expose it directly, or make it private (e.g.,_lock) to encourage using the pre-commit hook instead? wdyt?
145-145: Newformat-fixtask. Consistent naming withformat-check—nice! Should we also rename_format-fix-ruffto_format-fix-ruff(already consistent) or consider aligning its naming more explicitly with Poe conventions? wdyt?
150-150: Rename lint task tolint. Swapping from_lint-rufftolintimproves discoverability and consistency.
160-160: Private lockfile check task. Renamingcheck-lockfileto_check-lockfilealigns with the pre-commit hook—nice consistency.
176-176: Verifycheck-localtask coverage. This runsunit-test-with-covbut skipsformat-check; was that intentional to speed up local dev cycles? wdyt?
bnchrch
left a comment
bnchrch
left a comment
There was a problem hiding this comment.
Love this! Ill leave it to someone whos impacted by this to approve. (cc: @aaronsteers @brianjlai @aldogonzalez8 etc...)
aaronsteers
left a comment
aaronsteers
left a comment
There was a problem hiding this comment.
This is awesome. Thank you, @pnilan!
QQ - do you find that one "run-me before pushing" format/lint fix step?
Should we add something like a poe pre-commit alias as a slightly more concise way to run pre-commit run --all-files?
aaronsteers
left a comment
aaronsteers
left a comment
There was a problem hiding this comment.
✅ Approved! One comment/question, non-blocking. Approving since this is already a improvement even without further changes.
What
pre-commit
prettierpre-commit hook w/ poe taskpyproject
check-lockfilepoe task private__init__isort exclusion due to resolved issue: Bug: Root__init__.pyimport and unstable and cannot be sorted due to circular references #12Other Notes
I originally set out to clean up the poe tasks but found that I do think most of these are useful/necessary. So my primary suggestion is:
pre-commitas the primary source of truth -- this should align with the CI checks (as of now). Greenpre-commitshould mean green CI checks.pyproject, Look at the available tasks and their respective descriptions by runningpoetry run poe. You'll get a nice list:Summary by CodeRabbit
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.