Repository navigation
Modernize Python tooling (uv, pyproject.toml, src layout, semantic release) - #409
salman2013 wants to merge 11 commits into
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…kflow Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for the pull request, @salman2013! This repository is currently unmaintained. 🔘 Find a technical reviewerTo get help with finding a technical reviewer, reach out to the community contributions project manager for this PR:
Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
irfanuddinahmad
left a comment
There was a problem hiding this comment.
Verified against actual current file content, PyPI, and this repo's real git tags -- not just the diff. All CI is green, which is expected since these bugs aren't caught by CI (release.yml only runs on push to master; the Makefile targets below aren't exercised by any CI job).
Same changelog: "false" bug as edx-lint#563 and code-annotations#246 -- identical release.yml pattern across all three, looks template-derived rather than repo-specific.
Separately, worth a discussion rather than a code fix: I couldn't find an explicit "unmaintained" declaration anywhere (no CODEOWNERS, repo isn't archived), but the signals are real -- sparse commit cadence, 27 open issues, and this PR itself has zero reviewers despite rewiring the release/publish pipeline. Given items below would each independently break the first automated release, I'd get explicit maintainer sign-off before enabling fully-automated PyPI publishing here, not just before configuring the PyPI trusted publisher.
What's done correctly and worth not re-litigating: all 6 action SHA pins are genuine (verified each via the commits API, including two that needed resolving an annotated tag object to its real commit), license/SPDX handling, OIDC-only publish scoping, __version__ -> importlib.metadata with the correct PyPI distribution name, and the pre-flight tag/PyPI version check (latest tag v3.0.0 matches PyPI's real latest 3.0.0).
| with: | ||
| git_committer_name: "github-actions" | ||
| git_committer_email: "github-actions@github.com" | ||
| changelog: "false" |
There was a problem hiding this comment.
changelog: "false" overrides the [tool.semantic_release.changelog] config in pyproject.toml -- silently disables changelog generation entirely despite the config looking correct. This already caused real CHANGELOG.rst content loss in a sibling repo in this effort.
| fallback_version = "0.0.0.dev0" | ||
|
|
||
| [tool.semantic_release] | ||
| tag_format = "{version}" |
There was a problem hiding this comment.
tag_format = "{version}" (bare) doesn't match this repo's actual tag convention. Checked gh api repos/openedx/api-doc-tools/tags: the last 9 releases all use a v prefix (v3.0.0, v2.1.2, v2.1.1, v2.1.0, v2.0.0, v1.8.0, v1.7.0, v1.6.0, v1.5.1) -- only releases at or below 1.5.0 are bare. With this setting, PSR's tag-matching regex won't recognize v3.0.0 (the actual latest, matching PyPI) as a prior release at all, and will treat 1.5.0 as the latest match instead. The first automated release would then compute its next version from 1.5.0 territory, well below the real 3.0.0 baseline. Should be tag_format = "v{version}".
|
|
||
| coverage: clean ## generate and view HTML coverage report | ||
| pytest --cov-report html | ||
| $(BROWSER)htmlcov/index.html |
There was a problem hiding this comment.
The coverage target above (line 28: pytest --cov-report html) and diff_cover below call pytest/diff-cover bare, without the uv run --group <name> prefix used consistently everywhere else in this Makefile (test:, style:, isort:, pylint: all correctly use it). On a fresh uv sync --group dev checkout these will fail with command-not-found -- neither tool is even declared in any dependency-group anymore (checked all six groups in pyproject.toml; the old requirements/dev.in had both diff-cover and edx-i18n-tools, neither made it into the migration).
| uv run --group test pytest | ||
|
|
||
| diff_cover: test ## find diff lines that need test coverage | ||
| diff-cover coverage.xml |
There was a problem hiding this comment.
Same issue as the coverage target above -- diff-cover isn't declared in any dependency-group, so this fails command-not-found on a clean checkout. Recommend adding diff-cover back into an appropriate group (e.g. dev) and prefixing this with uv run --group dev.
|
|
||
| detect_changed_source_translations: | ||
| cd edx_api_doc_tools && i18n_tool changed | ||
| cd src/edx_api_doc_tools && i18n_tool changed |
There was a problem hiding this comment.
i18n_tool comes from edx-i18n-tools, which is likewise missing from every dependency-group after this migration (present in the old requirements/dev.in). This target, dummy_translations below, and anything depending on them (build_dummy_translations, validate_translations) will fail command-not-found on a clean uv sync --group dev checkout. Not caught by CI since these targets aren't part of any CI job.
| pull_request: | ||
| branches: | ||
| - "**" | ||
| workflow_call: |
There was a problem hiding this comment.
This new workflow_call: trigger, combined with the pre-existing push: [master] trigger a few lines up (unchanged by this PR), means every push to master now runs this full matrix twice concurrently: once via the direct push trigger, once via release.yml calling this workflow as its run_tests job. Recommend dropping the push: [master] trigger now that release.yml owns that path.
- Fix tag_format to v{version} to match repo's actual tag convention
- Enable changelog generation in release.yml (remove changelog: false)
- Drop push: [master] from ci.yml to avoid double runs via release.yml
- Fix coverage/diff_cover Makefile targets to use uv run --group
- Add diff-cover and edx-i18n-tools to dev dependency group
- Prefix i18n_tool calls in Makefile with uv run --group dev
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix tag_format to v{version} to match repo's actual tag convention
- Enable changelog generation in release.yml (remove changelog: false)
- Drop push: [master] from ci.yml to avoid double runs via release.yml
- Fix coverage/diff_cover Makefile targets to use uv run --group
- Add diff-cover and edx-i18n-tools to dev dependency group
- Prefix i18n_tool calls in Makefile with uv run --group dev
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tooling # Conflicts: # .github/workflows/ci.yml # .github/workflows/pypi-publish.yml
… review - Drop stale setuptools-scm fallback_version and 0.x-only semantic-release settings now that the repo has real tags through v3.0.0 - Add explicit pytest to test-base, missing build package to doc group (make build_docs was failing with "No module named build"), and the doc group to dev so docs tooling installs with `uv sync --group dev` - Add default-groups = [] so `uv sync --group ci` no longer silently pulls in the whole dev superset; add the missing [tool.edx_lint] table - Switch coverage source to source_pkgs for correct src-layout resolution - Remove MANIFEST.in references to the deleted requirements/ directory - Add contents:read permissions and --locked to ci.yml's uv sync/tox steps - Rewrite release.yml to match the current reference pattern: proper job/step naming and pins, fixed changelog/vcs_release/git_committer settings on the semantic-release step, and the missing "create GitHub Release with assets" step so dist files are attached before publish - Regenerate uv.lock to match the dependency-group changes above Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Makefile: call tools bare instead of prefixing with `uv run --group`, since that re-syncs the env on every invocation when tox calls these targets; `upgrade` now uses `uv run --with edx-lint edx_lint write_uv_constraints pyproject.toml` - pyproject.toml: drop the dead `[tool.semantic_release.changelog]` tables, since release.yml runs PSR with `changelog: "false"` - release.yml: re-sync structure with sample-plugin's current file (job comments, PSR tag-adjust comment, `id: artifact-download`, and rename the artifact to `distribution-artifacts` with `path: dist`) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Makefile: call tools bare instead of prefixing with `uv run --group`, since that re-syncs the env on every invocation when tox calls these targets; `upgrade` now uses `uv run --with edx-lint edx_lint write_uv_constraints pyproject.toml` - pyproject.toml: drop the dead `[tool.semantic_release.changelog]` tables, since release.yml runs PSR with `changelog: "false"` - release.yml: re-sync structure with sample-plugin's current file (job comments, PSR tag-adjust comment, `id: artifact-download`, and rename the artifact to `distribution-artifacts` with `path: dist`) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tooling # Conflicts: # .github/workflows/ci.yml # .github/workflows/pypi-publish.yml # requirements/common_constraints.txt # requirements/dev.txt # requirements/doc.txt # requirements/pip-tools.txt # requirements/pip.txt # requirements/quality.txt
irfanuddinahmad
left a comment
There was a problem hiding this comment.
Thanks, the October round is all in. A few more:
- Please reseed
uv.lockfrom master'srequirements/*.txtpins instead of a fresh resolve: runedx_lint write_uv_constraints pyproject.toml, add master's pins asname==vertoconstraint-dependencies,uv lock, restore pyproject,uv lockagain, then diffuv export --only-group <g>against eachrequirements/<g>.txt. Right now it differs in every group, e.g. Django 5.2.12 → 5.2.17, edx-lint 5.6.0 → 6.1.0, twine 6 → 7. - Please keep
__version__insrc/edx_api_doc_tools/__init__.py:from importlib.metadata import versionand__version__ = version("edx-api-doc-tools"). - Please use plain
Djangoindependencies, as PyPI 3.0.0 published. - Release settings, inline.
nits:
- CHANGELOG.rst: drop the insertion marker; nothing uses it with
changelog: "false". - README CI badge: point it at
actions/workflows/release.yml/badge.svg?branch=master, since ci.yml no longer runs on master. .tx/config:src/edx_api_doc_tools/conf/locale/....- Makefile:
python ../../manage.py ...instead of../../manage.py .... - The job name changed from
Testsand the env fromdjango52-drflatest; if either is a required status check, keep the old name. - Please retype the
fix: address review ...commits (e.g.build:), since each becomes a release-notes line on a merge commit.
| license-files = ["LICENSE.txt"] | ||
| requires-python = ">=3.12" | ||
| dependencies = [ | ||
| "Django>=4.2", |
There was a problem hiding this comment.
Please use plain Django, as PyPI 3.0.0 published.
| version_scheme = "only-version" | ||
| local_scheme = "no-local-version" | ||
|
|
||
| [tool.semantic_release] |
There was a problem hiding this comment.
Please add tag_format = "v{version}", major_on_zero = false and allow_zero_version = true here, and fallback_version = "0.0.0" under [tool.setuptools_scm].
- Reseed uv.lock from master's requirements/*.txt pins so the migration doesn't silently upgrade dependencies. - Restore __version__ via importlib.metadata. - Use plain Django in dependencies, matching the published 3.0.0. - Add major_on_zero, allow_zero_version and setuptools_scm fallback_version. - Align the dev group with master's dev.in (no doc group). - Drop the unused changelog insertion marker, point the README CI badge at release.yml, use src/ paths in .tx/config and invoke manage.py via python in the Makefile. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
98e3191 to
ca9815e
Compare
- Reseed uv.lock from master's requirements/*.txt pins so the migration doesn't silently upgrade dependencies. - Restore __version__ via importlib.metadata. - Use plain Django in dependencies, matching the published 3.0.0. - Add major_on_zero, allow_zero_version and setuptools_scm fallback_version. - Align the dev group with master's dev.in (no doc group). - Drop the unused changelog insertion marker, point the README CI badge at release.yml, use src/ paths in .tx/config and invoke manage.py via python in the Makefile. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ca9815e to
487f656
Compare
irfanuddinahmad
left a comment
There was a problem hiding this comment.
Thanks, all addressed. The lock now matches master's pins (the only difference is docutils, where master's doc.txt and quality.txt disagree).
Summary
setup.py/setup.cfgtopyproject.toml(PEP 621); add setuptools-scm for dynamic versioning; move coverage config intopyproject.toml; addpython-semantic-releasechangelog config; add.. changelog-insertion-markertoCHANGELOG.rstrequirements/withuvdependency groups anduv.lock; rewritetox.inito usetox-uv; updateMakefileto useuv run --groupcommands; update CI to useastral-sh/setup-uvand addworkflow_calltriggerpython-semantic-releaseworkflow (release.yml) with OIDC PyPI publishing; remove legacypypi-publish.ymlsrc/edx_api_doc_tools/(src layout); update all filesystem path referencesTicket: openedx/public-engineering#519
Test plan
make qualitypasses locally (pycodestyle, pydocstyle, isort, pylint all clean)uv run tox -e django52passesopenedx/api-doc-tools, workflowrelease.yml🤖 Generated with Claude Code