-
Notifications
You must be signed in to change notification settings - Fork 1
Migrate to pyproject.toml and synchronize readme with command defs #74
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
Conversation
WalkthroughThe changes migrate the Python project's build and packaging system from a traditional Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant CLI
participant BuildSystem(pyproject.toml)
participant PyPI
Developer->>CLI: Run build or install commands
CLI->>BuildSystem(pyproject.toml): Read project metadata and dependencies
BuildSystem(pyproject.toml)->>CLI: Provide build configuration
CLI->>PyPI: Publish package (via workflow)
Estimated code review effort2 (~15 minutes) Poem
✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
README.md (1)
52-54: Add an explicit step to installuvbeforeuv syncFirst-time contributors will hit
command not found: uvunless the tool is installed.
Consider inserting a one-liner such as:pipx install uv # or: pip install uvright before the
uv syncinvocation..github/workflows/ci.yaml (1)
26-34: Minor optimisation: cache the build layer & installwheel
python -m buildstill requireswheel; although it is pulled indirectly, installing it explicitly and caching
~/.cache/pipwill shave 15-20 s off every run.- name: Install build dependencies run: | python -m pip install --upgrade pip - pip install build twine + pip install wheel build twine - name: Cache pip uses: actions/cache@v4 with: path: ~/.cache/pip key: ${{ runner.os }}-pip-${{ hashFiles('pyproject.toml') }}pyproject.toml (1)
16-26: Pin critical runtime dependencies for reproducibility & supply chain securityAll runtime requirements are unpinned. At minimum, upper-bounds or compatible caps (
Click>=8,<9) help avoid sudden breaking
changes. This is especially important for CLI tools consumed in CI/CD.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci.yaml(1 hunks).gitignore(1 hunks)README.md(6 hunks)pyproject.toml(1 hunks)setup.py(0 hunks)
🧠 Learnings (1)
pyproject.toml (1)
Learnt from: vishnukv-facets
PR: Facets-cloud/module-development-cli#0
File: :0-0
Timestamp: 2025-05-26T13:13:07.039Z
Learning: All user-facing properties in spec must have both 'title' and 'description' fields. Properties with 'x-ui-override-disable': true are considered internal and skip this validation, while properties with 'x-ui-overrides-only': true are still user-facing and require validation. This is enforced by the check_properties_have_required_fields() function in ftf_cli/utils.py.
💤 Files with no reviewable changes (1)
- setup.py
🧰 Additional context used
🧠 Learnings (1)
pyproject.toml (1)
Learnt from: vishnukv-facets
PR: Facets-cloud/module-development-cli#0
File: :0-0
Timestamp: 2025-05-26T13:13:07.039Z
Learning: All user-facing properties in spec must have both 'title' and 'description' fields. Properties with 'x-ui-override-disable': true are considered internal and skip this validation, while properties with 'x-ui-overrides-only': true are still user-facing and require validation. This is enforced by the check_properties_have_required_fields() function in ftf_cli/utils.py.
🔇 Additional comments (3)
.gitignore (1)
82-84: Good addition – keepuv.lockout of VCSIgnoring
uv.lockavoids noisy diffs between machines and prevents accidental divergence of the lock-file frompyproject.toml.README.md (1)
141-146: New--skip-terraform-validationflag is well-documented
The updated notes accurately explain when validation is skipped.pyproject.toml (1)
28-34: Verifypyhclcompatibility with Python 3.11
pyhclhas historically lagged behind new Python versions. Double-check it installs cleanly on 3.11 or consider replacing it with
python-hcl2, which is actively maintained.
| [build-system] | ||
| requires = ["setuptools>=45", "wheel", "setuptools_scm[toml]>=6.2"] | ||
| build-backend = "setuptools.build_meta" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Potential versioning conflict: setuptools_scm vs dynamic.version
Including setuptools_scm[toml] implies you want SCM-based versions, yet [tool.setuptools.dynamic] version = {file = "VERSION"} overrides that mechanism.
Choose one source of truth to avoid ambiguous versions on PyPI.
🤖 Prompt for AI Agents
In pyproject.toml lines 1 to 4, there is a conflict between using setuptools_scm
for SCM-based versioning and the dynamic version set from a VERSION file. To fix
this, decide on a single versioning method: either remove setuptools_scm from
the requires list if you want to use the VERSION file approach, or remove the
dynamic.version configuration if you prefer setuptools_scm. Ensure only one
version source is configured to avoid ambiguity.
Fixes #73
Summary by CodeRabbit
New Features
Documentation
Chores