Modernize Python repo: pyproject.toml + uv + semantic-release - #388
Modernize Python repo: pyproject.toml + uv + semantic-release#388salman2013 wants to merge 2 commits into
Conversation
|
Thanks for the pull request, @salman2013! This repository is currently maintained by 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #388 +/- ##
==========================================
- Coverage 55.35% 53.44% -1.91%
==========================================
Files 3 2 -1
Lines 56 58 +2
Branches 0 2 +2
==========================================
Hits 31 31
- Misses 25 27 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
👋 Reviewed this against the same checklist we've been applying across the modernization effort. Nothing blocking — CI is fully green — just two small things worth a look: 1. 2. Non-blocking: |
f53a032 to
48fda12
Compare
4053301 to
11cd266
Compare
irfanuddinahmad
left a comment
There was a problem hiding this comment.
First review on this PR (no prior review activity). Verified everything against the actual current file content and live CI (all green) rather than just the diff. A few of these are the same recurring pattern already confirmed causing real breakage on sibling PRs in this same migration effort (fallback_version/fetch-depth, default-groups).
Things that are already done correctly and worth not re-litigating: all 6 action SHA pins are genuine (verified via the commits API, including one that needed resolving an annotated-tag object to its real commit), pypa/gh-action-pypi-publish correctly left at the stable tag, OIDC-only publish scoping, no License :: classifier conflicting with the SPDX field, src/ layout correctly adopted, CHANGELOG.rst's insertion marker present with no history dropped, and the Django version-matrix uses the correct [tool.uv].conflicts pattern rather than the shared-group anti-pattern found on 3 sibling repos.
| [tool.semantic_release] | ||
| build_command = "pip install build && SETUPTOOLS_SCM_PRETEND_VERSION=$NEW_VERSION python -m build" | ||
| allow_zero_version = true | ||
| major_on_zero = false |
There was a problem hiding this comment.
tag_format isn't set here, so it defaults to python-semantic-release's "v{version}". This repo's actual releases are bare X.Y.Z (3.0.0, 2.5.0, ... -- confirmed via the tags API, and 3.0.0 matches PyPI's actual latest published version). Left at the default, PSR won't recognize any prior release as such.
There's also a genuinely confusing existing tag worth flagging separately: v3.0.0 (with the prefix) exists too, but it points to a different, later commit (21c502fb, a routine "chore: Upgrade Python requirements" commit from 11 days after the real 3.0.0 release at c116322a) -- not something this PR should try to silently fix, but worth a maintainer's attention since it could confuse PSR's history scan regardless of tag_format.
| major_on_zero = false | |
| allow_zero_version = true | |
| major_on_zero = false | |
| tag_format = "{version}" |
| [tool.setuptools_scm] | ||
| version_scheme = "only-version" | ||
| local_scheme = "no-local-version" | ||
| fallback_version = "0.0.0.dev0" |
There was a problem hiding this comment.
"0.0.0.dev0" is the exact fallback value that crashed 17 tests in a sibling repo (openedx-events) via tuple(map(int, __version__.split("."))) choking on the non-numeric dev0 segment. I checked every __version__ consumer in this repo (src/done/__init__.py, src/done/done.py, docs/source/conf.py) -- none int-parse it, so it's not an active crash risk here today, but there's no reason to keep a value from the banned-pattern class when a plain int-parseable one works just as well.
| fallback_version = "0.0.0.dev0" | |
| fallback_version = "0.0.0" |
| toxenv: [django42, django52, quality] | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 |
There was a problem hiding this comment.
Missing fetch-depth: 0. Without it, this is a shallow, tag-less clone, so setuptools-scm can't see any tags during test runs and silently falls back to fallback_version (see the pyproject.toml comment) on every ordinary CI run. release.yml's own checkout correctly has this set; this job's doesn't.
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | |
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | |
| with: | |
| fetch-depth: 0 |
| from importlib.metadata import version | ||
|
|
||
| from .done import DoneXBlock | ||
|
|
There was a problem hiding this comment.
This should be wrapped in try/except PackageNotFoundError -- as written, any environment where done-xblock's dist-info isn't discoverable at import time (some editable-install/plugin-loading edge cases) raises unhandled and takes down the whole XBlock module, which is a harder failure than the hardcoded string this replaced.
| from importlib.metadata import PackageNotFoundError, version | |
| from .done import DoneXBlock | |
| try: | |
| __version__ = version("done-xblock") | |
| except PackageNotFoundError: | |
| __version__ = "unknown" |
There was a problem hiding this comment.
I removed this version as UV manages by own.
|
|
||
| install: install-test | ||
|
|
||
| quality: ## Run the quality checks |
There was a problem hiding this comment.
ruff is declared as a dependency in the quality group and fully configured ([tool.ruff]/[tool.ruff.lint]/[tool.ruff.format] in pyproject.toml), but this target never actually invokes it -- only pylint runs. Was ruff meant to run here too (matching the #511 XBlocks track's adoption of it elsewhere, e.g. xblocks-core), or was pylint intended to stay as the sole linter with ruff left over from an earlier draft? Either way it's currently dead config -- worth wiring in or dropping.
There was a problem hiding this comment.
We are not adding ruff in this scope because it needs to add more files formatting, so i removed that.
| changelog_file = "CHANGELOG.rst" | ||
| output_format = "rst" | ||
|
|
||
| [tool.uv] |
There was a problem hiding this comment.
A dev-named dependency-group exists elsewhere in this file, and every invocation in this repo (uv sync --group ci, uv sync --group dev, tox's dependency_groups =) already names an explicit group -- exactly the condition where default-groups = [] is needed. Without it, uv sync --group ci also implicitly syncs the full dev superset alongside whatever group was actually requested.
| [tool.uv] | |
| [tool.uv] | |
| package = true | |
| # Every `uv sync`/`uv run` invocation in this repo names an explicit --group. | |
| # Without this, uv's implicit default group (named "dev") would be synced *in | |
| # addition* to whatever --group is passed, defeating the point of having | |
| # separate groups. | |
| default-groups = [] | |
| conflicts = [ | |
| [{group = "test"}, {group = "django42"}], | |
| ] |
| name = "done-xblock" | ||
| description = "done XBlock" | ||
| readme = "README.rst" | ||
| license = "AGPL-3.0" |
There was a problem hiding this comment.
Minor: "AGPL-3.0" is a deprecated SPDX identifier (superseded by AGPL-3.0-only/AGPL-3.0-or-later). Not a build failure (no conflicting License :: classifier is present), just imprecise -- worth checking the LICENSE file's actual text to pick the right variant.
631547b to
fe96ae6
Compare
irfanuddinahmad
left a comment
There was a problem hiding this comment.
A couple of minor, non-blocking notes.
| quality: ## Run the quality checks | ||
| pylint --rcfile=pylintrc done | ||
| python setup.py -q sdist | ||
| pylint --rcfile=pylintrc src/done |
There was a problem hiding this comment.
These (and test/covreport below) call pylint/python/twine bare, no uv run — only bites if someone runs them directly instead of through tox. Same assumption existed pre-migration too, so not a new issue, just worth a follow-up sometime.
| python: | ||
| install: | ||
| - requirements: requirements/docs.txt | ||
| - method: pip |
There was a problem hiding this comment.
Heads up, RTD now has native uv support (method: uv, command: sync) if you ever want doc deps resolved from uv.lock too — not necessary for just Sphinx + the theme though, this is fine as is.
a507202 to
c5a89c5
Compare
|
|
||
| from .done import DoneXBlock | ||
|
|
||
| __version__ = '3.0.0' |
There was a problem hiding this comment.
This should be replaced with a get_version call and the variable should still be set for convenience/compatibility.
There was a problem hiding this comment.
I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup.
There was a problem hiding this comment.
You're right — python-semantic-release can't auto-update it in our current setup . Removed the file and the
[tool.semantic_release.changelog] config block from pyproject.toml as well, which auto-generated this file.
There was a problem hiding this comment.
Is this file new or is this config being moved from somewhere else, I don't see the file this is coming from if it's moving.
There was a problem hiding this comment.
Yes this is a new addition, just added in parity of other repo like xblock-core to show the code coverage in the checks list. should i remove this? as it is not a requirement of modernization.
There was a problem hiding this comment.
Yes, codecov has default configuration which is fine in most cases. We only need to override it sometimes.
There was a problem hiding this comment.
I have removed it.
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
Set changelog: "false" on the PSR release step and remove the [tool.semantic_release.changelog] config / insertion marker. Checked against openedx/XBlock's actual production release.yml (the one repo in this effort that has cut real automated releases) -- every run passes changelog: false and invokes `semantic-release -v version --no-changelog`, and the repo has zero github-actions[bot] commits ever. The auto-changelog config this migration previously added was only ever verified via a local dry-run prototype, never against a real release. feanil flagged the same issue on openedx/DoneXBlock#388: "I thought we were not going to add changelogs since they can't be updated by python-semantic-release the way we have it setup."
b13eaab to
fd87ebd
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
7be658b to
720583e
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@feanil I believe its ready for another pass. |
Modernizes the Python tooling for
DoneXBlockin three phases, aligning with the Open edX org standard established inopenedx/sample-plugin.Ticket: openedx/public-engineering#511
Parent ticket: openedx/public-engineering#506