! r always canonicalize version_number
#232
Merged
Conversation
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
Reviewer's GuideThis PR centralizes and canonicalizes version number handling by introducing a dedicated script to generate normalized version.py files from a git tag, removing ad-hoc normalization logic from tests and setup, and updating CI to use the new approach. Sequence diagram for canonicalized version setting in PyPI publish workflowsequenceDiagram
participant GitHubActions
participant PublishWorkflow
participant SetVersionStep
participant SetVersionScript
participant VersionPy as version_py
participant ApprovalVersionPy as approvaltests_version_py
GitHubActions->>PublishWorkflow: trigger workflow on tag
PublishWorkflow->>SetVersionStep: run step set version
SetVersionStep->>SetVersionScript: python setup/set_version.py tag
SetVersionScript->>SetVersionScript: parse tag as Version
SetVersionScript->>SetVersionScript: normalize to canonical version string
SetVersionScript->>VersionPy: write version_number = "<canonical>"
SetVersionScript->>ApprovalVersionPy: write version_number = "<canonical>"
SetVersionScript-->>SetVersionStep: exit success
SetVersionStep-->>PublishWorkflow: version files updated
PublishWorkflow-->>GitHubActions: continue with build and publish
Updated class diagram for version handling utilitiesclassDiagram
class setup_utils {
+get_version() str
}
class set_version {
+_VERSION_FILES
+main() void
}
class version_py {
+version_number str
}
class approvaltests_version_py {
+version_number str
}
setup_utils --> version_py : imports version_number
set_version --> version_py : writes version_number
set_version --> approvaltests_version_py : writes version_number
approvaltests_version_py <.. version_py : parallel version source
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
setup/set_version.py, consider making_VERSION_FILESan explicit list or tuple ofPathobjects rather than amap(Path, [...])for readability and easier inspection/debugging. - The paths in
_VERSION_FILESare currently relative to the working directory; to make the script more robust when run from other locations, you could derive these paths fromPath(__file__).parent.parent(project root) instead of assumingcwd.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `setup/set_version.py`, consider making `_VERSION_FILES` an explicit list or tuple of `Path` objects rather than a `map(Path, [...])` for readability and easier inspection/debugging.
- The paths in `_VERSION_FILES` are currently relative to the working directory; to make the script more robust when run from other locations, you could derive these paths from `Path(__file__).parent.parent` (project root) instead of assuming `cwd`.
## Individual Comments
### Comment 1
<location> `test__mypy_accepts_our_packages.py:12-13` </location>
<code_context>
-
from version import version_number
-# normalize version number
-version_number = str(Version(version_number))
-
-
</code_context>
<issue_to_address>
**suggestion (testing):** Add an explicit test that `version_number` is already canonicalized instead of relying on implicit normalization here
Now that normalization happens in the version-generation step, this test no longer verifies that `version.version_number` is canonical. Please add a focused test (e.g., `test_version_is_canonical`) that imports `version_number` from `version.py` and asserts that:
- it can be parsed by `packaging.version.Version`
- it has no leading `v`
- it matches the expected normalized string for a fixed input (e.g., via mocking or a fixed tag)
This makes the canonicalization contract explicit and guards against regressions where a non-canonical string slips through while this test still passes.
Suggested implementation:
```python
import time
import typing
from packaging.version import Version
from version import version_number
```
```python
from version import version_number
def test_version_is_canonical() -> None:
parsed = Version(version_number)
# Should be parseable and already in normalized form
assert not version_number.startswith("v")
assert version_number == str(parsed)
def main() -> None:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
-12
to
-13
| # normalize version number | ||
| version_number = str(Version(version_number)) |
There was a problem hiding this comment.
suggestion (testing): Add an explicit test that version_number is already canonicalized instead of relying on implicit normalization here
Now that normalization happens in the version-generation step, this test no longer verifies that version.version_number is canonical. Please add a focused test (e.g., test_version_is_canonical) that imports version_number from version.py and asserts that:
- it can be parsed by
packaging.version.Version - it has no leading
v - it matches the expected normalized string for a fixed input (e.g., via mocking or a fixed tag)
This makes the canonicalization contract explicit and guards against regressions where a non-canonical string slips through while this test still passes.
Suggested implementation:
import time
import typing
from packaging.version import Version
from version import version_numberfrom version import version_number
def test_version_is_canonical() -> None:
parsed = Version(version_number)
# Should be parseable and already in normalized form
assert not version_number.startswith("v")
assert version_number == str(parsed)
def main() -> None:
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.
Previous Behavior
version.version_numberincluded avprefix and could use-or.; code that usedversion_numberwould have to clean this up. Inconsistent cleanup has been a source of bugs.New behavior
When writing
version.py, always canonicalize the value before writing it, so that all consuming code can just use it as-is.Summary by Sourcery
Canonicalize the project version number at the time version files are written so all consumers can use a normalized version string without additional cleanup.
New Features:
Enhancements: