VersionCheck: compare base branch instead of registry; drop @assert - #65
Merged
Merged
Conversation
The previous check pulled the latest registered version from
ITensorRegistry and asserted current > registered. This had a design
gap: between a substantive PR merging (with a version bump) and the
new version actually being registered, any number of further
substantive PRs would pass without bumping the version, because
"registered" was still the older value.
Switch to comparing the current branch's Project.toml version
against the same file on the PR's base branch. That answers the
question reviewers actually have ("did this PR move the version
forward relative to main"), removes the registration-timing
dependency, and matches how related ecosystems (e.g.
JuliaRegistries) intuit the check.
Implementation:
- Add a "Fetch base branch" step (the default actions/checkout@v4 is
shallow on PR head only, so the base branch isn't otherwise
available) that does git fetch --depth=1 origin "$GITHUB_BASE_REF".
- Drop julia-actions/julia-buildpkg from this workflow — the new check
only needs TOML (stdlib), not registry resolution.
- Use TOML.parse + VersionNumber + a real if/error rather than @Assert,
so the failure produces a clean Julia error message rather than an
AssertionError. The error spells out the base ref and both versions.
- Skip gracefully when the base Project.toml is missing (initial-commit
edge case) or when neither file has a version field. Error if only
the PR removed the version field (regression).
The localregistry input becomes unused but is kept (with an updated
description) for backward compatibility with existing callers in
ITensorPkgSkeleton-generated workflows.
Tracked in
ITensorDevelopmentPlans/Projects/ITensorActions/fixes_and_improvements.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
VersionCheckpreviously pulled the latest registered version fromITensorRegistryand assertedregistered_version < current_version. That has a design gap: between a substantive PR merging (with a version bump) and the new version being registered, any number of further substantive PRs pass without bumping the version becauseregistered_versionis still the old value.Concrete example seen on ITensor/ITensorGaussianMPS.jl#24:
Project.tomlProject.tomlOld check:
0.1.13 < 0.1.14→ pass ✅. Reviewer intuition: ❌, the PR didn't move the version forward.Fix
Compare the current branch's
Project.tomlversion against the same file on the PR's base branch. Answers the question reviewers actually have ("did this PR move the version forward relative to main"), removes the registration-timing dependency, and matches how General-registry tooling thinks about the check.Implementation
Fetch base branchstep (actions/checkout@v4is shallow on PR head only; the base branch isn't otherwise available):git fetch --depth=1 origin "$GITHUB_BASE_REF".julia-actions/julia-buildpkgfrom this workflow — the new check only needsTOML(stdlib), not registry resolution.TOML.parse+VersionNumber+ a realif/error("…")rather than@assert, so the failure produces a clean Julia error rather than anAssertionError. The error message spells out the base ref and both versions for actionable feedback.Project.tomlis missing (initial-commit edge case) or when neither file has aversionfield. Error if only the PR removed the version field.The
localregistryinput becomes unused but is kept (with an updated description) for backward compatibility with existing callers in ITensorPkgSkeleton-generated workflows.Test plan
Project.tomlversion reports a clear failure message naming both versions..github/**-only changes) still skip via theclassify-praction.🤖 Generated with Claude Code