-
Notifications
You must be signed in to change notification settings - Fork 259
add (optional) size and upload_time to link and Package.files #905
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideAdds optional size and upload_time metadata to Link objects and Package.files entries, wiring through initialization, typed dict structure, and providing parsed datetime accessors plus tests for the new behavior. Class diagram for updated Link and PackageFile structuresclassDiagram
class Link {
-url str
-_metadata str|bool|dict~str,str~|None
-_yanked str|bool
-_size int|None
-_upload_time str|None
+__init__(url str, requires_python str|None, yanked str|bool, hashes Mapping~str,str~|None, metadata str|bool|dict~str,str~|None, size int|None, upload_time str|None) void
+__str__() str
+yanked() bool
+yanked_reason() str
+size() int|None
+upload_time_isoformat() str|None
+upload_time() datetime|None
}
class _PackageFile {
+file str
+hash str
}
class PackageFile {
+file str
+hash str
+url str
+size int
+upload_time str
}
class Package {
+files list~PackageFile~
}
PackageFile --|> _PackageFile : extends
Package "1" --> "*" PackageFile : uses as files
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- The
test_package_link_size_default_Nonetest name is misleading since it setssize=1234and asserts that value; consider renaming it (e.g.test_package_link_size_explicit_value) to better reflect its purpose. - In
Link.upload_time, usingself._upload_time.replace("Z", "+00:00")will replace allZcharacters, not just a trailing UTC designator; it would be safer to normalize only a terminalZ(e.g. withendswith("Z")) before callingdatetime.fromisoformat.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `test_package_link_size_default_None` test name is misleading since it sets `size=1234` and asserts that value; consider renaming it (e.g. `test_package_link_size_explicit_value`) to better reflect its purpose.
- In `Link.upload_time`, using `self._upload_time.replace("Z", "+00:00")` will replace all `Z` characters, not just a trailing UTC designator; it would be safer to normalize only a terminal `Z` (e.g. with `endswith("Z")`) before calling `datetime.fromisoformat`.
## Individual Comments
### Comment 1
<location> `tests/packages/utils/test_utils_link.py:3-9` </location>
<code_context>
+ assert link.upload_time is None
+
+
[email protected](
+ ("upload_time", "expected"),
+ [
+ ("2023-06-15T08:30:45Z", datetime(2023, 6, 15, 8, 30, 45, tzinfo=timezone.utc)),
+ (
+ "2022-12-31T23:59:59+00:00",
+ datetime(2022, 12, 31, 23, 59, 59, tzinfo=timezone.utc),
+ ),
+ (
+ "2023-06-15T08:30:45.123456Z",
+ datetime(2023, 6, 15, 8, 30, 45, 123456, tzinfo=timezone.utc),
+ ),
+ ],
+)
+def test_package_link_upload_time(upload_time: str, expected: datetime) -> None:
+ link = Link("https://example.org", upload_time=upload_time)
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for invalid and offsetted `upload_time` values
The current parametrized test nicely covers several valid ISO 8601 formats. To strengthen it, please also add cases for (1) an invalid ISO string (e.g. "not-a-timestamp") to assert `link.upload_time` is `None`, and (2) a timestamp with a non-zero offset (e.g. `"2023-06-15T10:30:45+02:00"`) so offset handling and behavior on older Python versions are exercised and guarded against regressions.
```suggestion
import uuid
from datetime import datetime
from datetime import timezone
from hashlib import sha256
import pytest
@pytest.mark.parametrize(
("upload_time", "expected"),
[
(
"2023-06-15T08:30:45Z",
datetime(2023, 6, 15, 8, 30, 45, tzinfo=timezone.utc),
),
(
"2022-12-31T23:59:59+00:00",
datetime(2022, 12, 31, 23, 59, 59, tzinfo=timezone.utc),
),
(
"2023-06-15T08:30:45.123456Z",
datetime(2023, 6, 15, 8, 30, 45, 123456, tzinfo=timezone.utc),
),
(
"2023-06-15T10:30:45+02:00",
datetime(2023, 6, 15, 8, 30, 45, tzinfo=timezone.utc),
),
("not-a-timestamp", None),
],
)
def test_package_link_upload_time(upload_time: str, expected: object) -> None:
link = Link("https://example.org", upload_time=upload_time)
assert link.upload_time == expected
assert link.yanked == expected_yanked
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
d98dadd to
4f0b458
Compare
4f0b458 to
c206e05
Compare
Related-to: python-poetry/poetry-plugin-export#336
Related-to: python-poetry/poetry#10356
Related-to: python-poetry/poetry#10646
Summary by Sourcery
Add optional distribution file size and upload timestamp metadata to links and package files.
New Features:
Tests: