feat: allow publishing artifacts if version is determined dynamically #10644
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 GuideAdjusts Poetry’s publish flow so that artifacts are selected based on the latest built distribution version on disk rather than the version in pyproject.toml, updates uploader behavior and messaging to use this detected version, refines registration behavior, and documents the new behavior, with accompanying tests. Sequence diagram for dynamic version detection during publishsequenceDiagram
actor User
participant PublishCommand
participant Publisher
participant Uploader
participant FileSystem
User ->> PublishCommand: run poetry publish --build
PublishCommand ->> PublishCommand: determine dist_dir
PublishCommand ->> PublishCommand: call build with --output dist_dir
PublishCommand ->> Publisher: create Publisher(poetry, io, dist_dir)
Publisher ->> Uploader: create Uploader(poetry, io, dist_dir)
PublishCommand ->> Publisher: publish(repository, username, password)
Publisher ->> Uploader: access files
activate Uploader
Uploader ->> FileSystem: list dist_dir matching dist_name-*-*.whl
Uploader ->> FileSystem: list dist_dir matching dist_name-*.tar.gz
Uploader ->> Uploader: group artifacts by parsed version
Uploader ->> Uploader: select latest version via Version.parse
Uploader ->> Uploader: sort artifacts (prefer wheels)
Uploader -->> Publisher: return files and version
deactivate Uploader
Publisher ->> Publisher: log package pretty_name and uploader.version
loop for each file
Publisher ->> Uploader: _register(session, url) or upload
Uploader ->> FileSystem: open file
Uploader -->> Publisher: upload response
end
Publisher -->> PublishCommand: publish result
PublishCommand -->> User: show publish summary with detected version
Class diagram for dynamic artifact selection in publish flowclassDiagram
class PublishCommand {
+handle() int
}
class Publisher {
-Poetry _poetry
-IO _io
-Uploader _uploader
-Package _package
+Publisher(poetry, io, dist_dir)
+publish(repository, username, password) int
}
class Uploader {
-Poetry _poetry
-string _dist_name
-IO _io
-Path _dist_dir
-string _username
-string _password
+Uploader(poetry, io, dist_dir)
+dist_dir Path
+files list~Path~
+version string
+auth(username, password) void
+_register(session, url) Response
+post_data(file) dict
+_files_and_version tuple~list~Path~~, string~
}
PublishCommand --> Publisher : uses
Publisher --> Uploader : composes
Uploader --> Poetry : uses
Uploader --> IO : uses
Publisher --> Package : reads
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:
- Using a cached_property for
_files_and_versionmeans the file list and version are frozen at first access; if new artifacts are added to the dist directory later in the same process (outside the explicit re-creation inpublish), the uploader may operate on stale data, so consider either avoiding caching or tying the cache invalidation explicitly to build/publish lifecycle events. - The version extraction logic in
_files_and_versionrelies on specific filename patterns and string operations onstem(including.removesuffix('.tar')and a split on-), which may be brittle for alternative distribution formats or unusual version tags; it might be safer to centralize and/or unit-test this parsing against a broader set of real-world filenames. - In
_register, usingself.files[0]assumes that at least one artifact exists and that the first one is appropriate for registration; it might be clearer and more robust to explicitly select an sdist if present and to fail fast with a descriptive error if no matching artifacts are found.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using a cached_property for `_files_and_version` means the file list and version are frozen at first access; if new artifacts are added to the dist directory later in the same process (outside the explicit re-creation in `publish`), the uploader may operate on stale data, so consider either avoiding caching or tying the cache invalidation explicitly to build/publish lifecycle events.
- The version extraction logic in `_files_and_version` relies on specific filename patterns and string operations on `stem` (including `.removesuffix('.tar')` and a split on `-`), which may be brittle for alternative distribution formats or unusual version tags; it might be safer to centralize and/or unit-test this parsing against a broader set of real-world filenames.
- In `_register`, using `self.files[0]` assumes that at least one artifact exists and that the first one is appropriate for registration; it might be clearer and more robust to explicitly select an sdist if present and to fail fast with a descriptive error if no matching artifacts are found.
## Individual Comments
### Comment 1
<location> `tests/publishing/test_uploader.py:35-42` </location>
<code_context>
+ return Uploader(poetry, NullIO())
+
+
[email protected](
+ ("files", "expected_files", "expected_version"),
+ [
+ (
+ ["simple_project-1.2.3.tar.gz", "simple_project-1.2.3-py3-none-any.whl"],
+ ["simple_project-1.2.3.tar.gz", "simple_project-1.2.3-py3-none-any.whl"],
+ "1.2.3",
+ ),
+ ( # other names are ignored
+ [
</code_context>
<issue_to_address>
**suggestion (testing):** Extend `test_uploader_files_only_latest` parametrization to cover empty dist and pre-release version ordering.
To exercise `_files_and_version` more thoroughly, consider adding two parameter sets:
1. Empty dist directory: no matching artifacts, expecting `uploader.files == []` and `uploader.version == ""` to cover the `case 0` branch.
2. Pre-release vs final: e.g. `1.2.3rc1` and `1.2.3`, asserting that `1.2.3` is selected to confirm `Version.parse` ordering and correct handling of pre-releases in the new dynamic resolution.
These will round out the spec for the artifact selection logic.
Suggested implementation:
```python
@pytest.fixture
def uploader(poetry: Poetry) -> Uploader:
return Uploader(poetry, NullIO())
@pytest.mark.parametrize(
("files", "expected_files", "expected_version"),
[
(
["simple_project-1.2.3.tar.gz", "simple_project-1.2.3-py3-none-any.whl"],
["simple_project-1.2.3.tar.gz", "simple_project-1.2.3-py3-none-any.whl"],
"1.2.3",
),
( # empty dist directory -> no files, no version
[],
[],
"",
),
( # pre-release vs final: final wins
[
"simple_project-1.2.3rc1.tar.gz",
"simple_project-1.2.3rc1-py3-none-any.whl",
"simple_project-1.2.3.tar.gz",
"simple_project-1.2.3-py3-none-any.whl",
],
[
"simple_project-1.2.3.tar.gz",
"simple_project-1.2.3-py3-none-any.whl",
],
"1.2.3",
),
( # other names are ignored
[
"simple_project-1.2.3.tar.gz",
"other_project-1.2.3.tar.gz",
"simple_project-1.2.3-py3-none-any.whl",
```
I assumed that:
- These parametrized values are consumed by `test_uploader_files_only_latest` (or similar) and that `expected_files` and `expected_version` are asserted against `uploader.files` and `uploader.version`.
- The empty dist case is represented by passing an empty `files` list into the test, which should exercise the branch where `_files_and_version` yields no version (`""`) and an empty file list.
- The pre-release vs final case is meant to assert that when both `1.2.3rc1` and `1.2.3` artifacts exist, only the `1.2.3` artifacts are selected, confirming that `Version.parse` ranks the final release higher than the pre-release.
If the test currently derives `files` from an actual directory rather than directly from this `files` argument, you may need to:
1. Ensure the test uses the `files` param to set up the dist directory contents (e.g. by touching files in a tmp path).
2. Confirm that the no-files case leads to `uploader.version == ""` (or adjust `expected_version` if the implementation uses `None` or some other sentinel).
</issue_to_address>
### Comment 2
<location> `tests/publishing/test_uploader.py:194-203` </location>
<code_context>
def test_uploader_registers_for_appropriate_400_errors(
- mocker: MockerFixture, http: responses.RequestsMock, uploader: Uploader
+ http: responses.RequestsMock, uploader: Uploader
) -> None:
- register = mocker.patch("poetry.publishing.uploader.Uploader._register")
http.post("https://foo.com", status=400, body="No package was ever registered")
with pytest.raises(UploadError):
uploader.upload("https://foo.com")
- assert register.call_count == 1
+ assert len(http.calls) == 2
+ assert b'name=":action"\r\n\r\nfile_upload\r\n' in (
+ http.calls[0].request.body or b""
+ )
+ assert b'name=":action"\r\n\r\nsubmit\r\n' in (http.calls[1].request.body or b"")
</code_context>
<issue_to_address>
**suggestion (testing):** Add a dedicated test for `_register` behavior when only wheel artifacts are present.
The new assertions on `http.calls` improve verification of the registration flow, but `_register` now uses `self.files[0]` instead of a specific sdist path. This changes behavior when only wheels exist: previously `_register` failed due to missing sdist; now it will register using the first file (a wheel).
Please add a dedicated test (e.g. `test_uploader_register_uses_wheel_if_no_sdist`) that:
- populates `dist_dir` with only a wheel for the latest version,
- triggers `_register` (directly or via the 400 upload flow), and
- asserts that the wheel is used (via `post_data` or `http.calls`).
This will document and lock in the new behavior when no sdist is available.
Suggested implementation:
```python
def test_uploader_properly_handles_400_errors(
def test_uploader_register_uses_wheel_if_no_sdist(
http: responses.RequestsMock,
poetry: "Poetry",
tmp_path: "Path",
) -> None:
wheel = tmp_path / "foo-1.2.3-py3-none-any.whl"
wheel.write_bytes(b"wheel-binary")
uploader = Uploader(poetry, NullIO(), dist_dir=tmp_path)
http.post("https://foo.com", status=400, body="No package was ever registered")
with pytest.raises(UploadError):
uploader.upload("https://foo.com")
assert len(http.calls) == 2
wheel_name = wheel.name.encode()
assert wheel_name in (http.calls[0].request.body or b"")
assert wheel_name in (http.calls[1].request.body or b"")
if TYPE_CHECKING:
```
This new test assumes:
1. `Uploader`, `UploadError`, `NullIO`, and `pytest` are already imported at the top of `tests/publishing/test_uploader.py`, consistent with the rest of the test file.
2. The `http` fixture of type `responses.RequestsMock`, the `poetry` fixture of type `Poetry`, and the `tmp_path` fixture are already available (which is typical for this test module).
3. The registry URL `"https://foo.com"` matches the one used in the other uploader tests, so that the `responses` mock intercepts the calls.
If the actual filename pattern for the wheel differs (e.g. project name or version), adjust `"foo-1.2.3-py3-none-any.whl"` accordingly to match what `poetry` produces for the latest version in your fixtures.
</issue_to_address>
### Comment 3
<location> `tests/publishing/test_publisher.py:53` </location>
<code_context>
config: Config,
fixture_name: str,
) -> None:
+ mocker.patch("poetry.publishing.uploader.Uploader.version", "1.2.3")
uploader_auth = mocker.patch("poetry.publishing.uploader.Uploader.auth")
uploader_upload = mocker.patch("poetry.publishing.uploader.Uploader.upload")
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen publisher test to assert the printed version matches the uploader's dynamic version.
Patching `Uploader.version` stabilizes the version used in the test, but it doesn’t yet verify the new behavior. Please extend this test (or add a sibling) to assert on the captured console output after running the command—for example, that it includes `Publishing simple-project (1.2.3) to ...`—so we confirm the publisher is actually using `self._uploader.version` rather than the static package version.
Suggested implementation:
```python
config: Config,
fixture_name: str,
capsys: CaptureFixture[str],
) -> None:
mocker.patch("poetry.publishing.uploader.Uploader.version", "1.2.3")
uploader_auth = mocker.patch("poetry.publishing.uploader.Uploader.auth")
uploader_upload = mocker.patch("poetry.publishing.uploader.Uploader.upload")
# After running the publish command, ensure we printed the uploader's version
captured = capsys.readouterr()
assert "Publishing simple-project (1.2.3) to " in captured.out
```
To make this compile and behave correctly, a couple of surrounding adjustments are likely needed:
1. **Import the capture fixture type**
At the top of `tests/publishing/test_publisher.py`, add:
```python
from pytest import CaptureFixture
```
(or `import pytest` and then use `pytest.CaptureFixture[str]` in the signature, depending on existing conventions in this file).
2. **Place the assertion after the publish command is executed**
The assertion added above must run *after* whatever code actually triggers the publishing (e.g., a CLI runner, command tester, or direct `Publisher.publish()` call).
If the publish command is currently invoked *after* the snippet you provided, you should move the `captured = capsys.readouterr()` and `assert ...` lines to immediately after that invocation. For example:
```python
# existing code that triggers the publish (example)
result = app.invoke(["publish"])
captured = capsys.readouterr()
assert "Publishing simple-project (1.2.3) to " in captured.out
```
3. **Ensure the project name matches**
If the fixture under test uses a different project name than `"simple-project"`, update the expected string to match the actual project name that appears in the publish message (keeping `(1.2.3)` as the version).
These adjustments will ensure the test both stabilizes the uploader version and verifies that the publisher’s user-facing output reflects `self._uploader.version`.
</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
+35
to
+42
| @pytest.mark.parametrize( | ||
| ("files", "expected_files", "expected_version"), | ||
| [ | ||
| ( | ||
| ["simple_project-1.2.3.tar.gz", "simple_project-1.2.3-py3-none-any.whl"], | ||
| ["simple_project-1.2.3.tar.gz", "simple_project-1.2.3-py3-none-any.whl"], | ||
| "1.2.3", | ||
| ), |
There was a problem hiding this comment.
suggestion (testing): Extend test_uploader_files_only_latest parametrization to cover empty dist and pre-release version ordering.
To exercise _files_and_version more thoroughly, consider adding two parameter sets:
- Empty dist directory: no matching artifacts, expecting
uploader.files == []anduploader.version == ""to cover thecase 0branch. - Pre-release vs final: e.g.
1.2.3rc1and1.2.3, asserting that1.2.3is selected to confirmVersion.parseordering and correct handling of pre-releases in the new dynamic resolution.
These will round out the spec for the artifact selection logic.
Suggested implementation:
@pytest.fixture
def uploader(poetry: Poetry) -> Uploader:
return Uploader(poetry, NullIO())
@pytest.mark.parametrize(
("files", "expected_files", "expected_version"),
[
(
["simple_project-1.2.3.tar.gz", "simple_project-1.2.3-py3-none-any.whl"],
["simple_project-1.2.3.tar.gz", "simple_project-1.2.3-py3-none-any.whl"],
"1.2.3",
),
( # empty dist directory -> no files, no version
[],
[],
"",
),
( # pre-release vs final: final wins
[
"simple_project-1.2.3rc1.tar.gz",
"simple_project-1.2.3rc1-py3-none-any.whl",
"simple_project-1.2.3.tar.gz",
"simple_project-1.2.3-py3-none-any.whl",
],
[
"simple_project-1.2.3.tar.gz",
"simple_project-1.2.3-py3-none-any.whl",
],
"1.2.3",
),
( # other names are ignored
[
"simple_project-1.2.3.tar.gz",
"other_project-1.2.3.tar.gz",
"simple_project-1.2.3-py3-none-any.whl",I assumed that:
- These parametrized values are consumed by
test_uploader_files_only_latest(or similar) and thatexpected_filesandexpected_versionare asserted againstuploader.filesanduploader.version. - The empty dist case is represented by passing an empty
fileslist into the test, which should exercise the branch where_files_and_versionyields no version ("") and an empty file list. - The pre-release vs final case is meant to assert that when both
1.2.3rc1and1.2.3artifacts exist, only the1.2.3artifacts are selected, confirming thatVersion.parseranks the final release higher than the pre-release.
If the test currently derives files from an actual directory rather than directly from this files argument, you may need to:
- Ensure the test uses the
filesparam to set up the dist directory contents (e.g. by touching files in a tmp path). - Confirm that the no-files case leads to
uploader.version == ""(or adjustexpected_versionif the implementation usesNoneor some other sentinel).
Comment on lines
194
to
+203
| def test_uploader_registers_for_appropriate_400_errors( | ||
| mocker: MockerFixture, http: responses.RequestsMock, uploader: Uploader | ||
| http: responses.RequestsMock, uploader: Uploader | ||
| ) -> None: | ||
| register = mocker.patch("poetry.publishing.uploader.Uploader._register") | ||
| http.post("https://foo.com", status=400, body="No package was ever registered") | ||
|
|
||
| with pytest.raises(UploadError): | ||
| uploader.upload("https://foo.com") | ||
|
|
||
| assert register.call_count == 1 | ||
| assert len(http.calls) == 2 | ||
| assert b'name=":action"\r\n\r\nfile_upload\r\n' in ( |
There was a problem hiding this comment.
suggestion (testing): Add a dedicated test for _register behavior when only wheel artifacts are present.
The new assertions on http.calls improve verification of the registration flow, but _register now uses self.files[0] instead of a specific sdist path. This changes behavior when only wheels exist: previously _register failed due to missing sdist; now it will register using the first file (a wheel).
Please add a dedicated test (e.g. test_uploader_register_uses_wheel_if_no_sdist) that:
- populates
dist_dirwith only a wheel for the latest version, - triggers
_register(directly or via the 400 upload flow), and - asserts that the wheel is used (via
post_dataorhttp.calls).
This will document and lock in the new behavior when no sdist is available.
Suggested implementation:
def test_uploader_properly_handles_400_errors(
def test_uploader_register_uses_wheel_if_no_sdist(
http: responses.RequestsMock,
poetry: "Poetry",
tmp_path: "Path",
) -> None:
wheel = tmp_path / "foo-1.2.3-py3-none-any.whl"
wheel.write_bytes(b"wheel-binary")
uploader = Uploader(poetry, NullIO(), dist_dir=tmp_path)
http.post("https://foo.com", status=400, body="No package was ever registered")
with pytest.raises(UploadError):
uploader.upload("https://foo.com")
assert len(http.calls) == 2
wheel_name = wheel.name.encode()
assert wheel_name in (http.calls[0].request.body or b"")
assert wheel_name in (http.calls[1].request.body or b"")
if TYPE_CHECKING:This new test assumes:
Uploader,UploadError,NullIO, andpytestare already imported at the top oftests/publishing/test_uploader.py, consistent with the rest of the test file.- The
httpfixture of typeresponses.RequestsMock, thepoetryfixture of typePoetry, and thetmp_pathfixture are already available (which is typical for this test module). - The registry URL
"https://foo.com"matches the one used in the other uploader tests, so that theresponsesmock intercepts the calls.
If the actual filename pattern for the wheel differs (e.g. project name or version), adjust "foo-1.2.3-py3-none-any.whl" accordingly to match what poetry produces for the latest version in your fixtures.
| config: Config, | ||
| fixture_name: str, | ||
| ) -> None: | ||
| mocker.patch("poetry.publishing.uploader.Uploader.version", "1.2.3") |
There was a problem hiding this comment.
suggestion (testing): Strengthen publisher test to assert the printed version matches the uploader's dynamic version.
Patching Uploader.version stabilizes the version used in the test, but it doesn’t yet verify the new behavior. Please extend this test (or add a sibling) to assert on the captured console output after running the command—for example, that it includes Publishing simple-project (1.2.3) to ...—so we confirm the publisher is actually using self._uploader.version rather than the static package version.
Suggested implementation:
config: Config,
fixture_name: str,
capsys: CaptureFixture[str],
) -> None:
mocker.patch("poetry.publishing.uploader.Uploader.version", "1.2.3")
uploader_auth = mocker.patch("poetry.publishing.uploader.Uploader.auth")
uploader_upload = mocker.patch("poetry.publishing.uploader.Uploader.upload")
# After running the publish command, ensure we printed the uploader's version
captured = capsys.readouterr()
assert "Publishing simple-project (1.2.3) to " in captured.outTo make this compile and behave correctly, a couple of surrounding adjustments are likely needed:
-
Import the capture fixture type
At the top oftests/publishing/test_publisher.py, add:from pytest import CaptureFixture
(or
import pytestand then usepytest.CaptureFixture[str]in the signature, depending on existing conventions in this file). -
Place the assertion after the publish command is executed
The assertion added above must run after whatever code actually triggers the publishing (e.g., a CLI runner, command tester, or directPublisher.publish()call).
If the publish command is currently invoked after the snippet you provided, you should move thecaptured = capsys.readouterr()andassert ...lines to immediately after that invocation. For example:# existing code that triggers the publish (example) result = app.invoke(["publish"]) captured = capsys.readouterr() assert "Publishing simple-project (1.2.3) to " in captured.out
-
Ensure the project name matches
If the fixture under test uses a different project name than"simple-project", update the expected string to match the actual project name that appears in the publish message (keeping(1.2.3)as the version).
These adjustments will ensure the test both stabilizes the uploader version and verifies that the publisher’s user-facing output reflects self._uploader.version.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Pull Request Check List
Until now,
poetry publishdid only upload artifacts that matched the name and the version in the pyproject.toml. This PR changes this constraint so that artifacts of the latest version in the dist directory are uploaded. (The name is still matched.) This is useful if some kind of dynamic versioning is used, e.g.:poetry build --local-versionSummary by Sourcery
Allow publishing the latest built distribution artifacts regardless of the static version in pyproject, improving support for dynamic versioning scenarios.
New Features:
Enhancements:
poetry publishto describe how artifacts are selected from the dist directory.Tests: