Skip to content

Add pylock #900

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Add pylock #900

wants to merge 13 commits into from

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented May 6, 2025

Here is the proposal for pylock.toml support.

  • immutable keywords-only dataclasses
  • a from_dict class method to validate and create a Pylock instance from a toml dict (obtained from tomllib.load)
  • a Pylock.to_dict method to convert to a spec-compliant toml dict (assuming types were respected when populating the dataclass), preserving the recommended field ordering
  • this uses frozen keyword-only dataclasses, which makes the constructors look weird, but that will be cleaned up without API change when this projects drops support for python 3.9
  • is_valid_pylock_path to validate pylock file names
  • toml (de)serialization left out on purpose

Documentation is still TODO.

closes #898

url: str | None # = None
path: str | None # = None
size: int | None # = None
upload_time: datetime | None # = None
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the spec, the upload_time field is not at the same position for archive and sdist / wheel.

Is that intended or something we want to tweak here and/or in the spec?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was intended as the idea (and expected value) comes from the index API and I don't think archives are supported there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if people have uses for an upload time for archives then I suspect making it a 1.1 change wouldn't be controversial.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note my remark was only about the ordering of the fields, not about the presence of upload_time in archive.

@sbidoul sbidoul marked this pull request as draft May 7, 2025 08:23
@sbidoul sbidoul marked this pull request as ready for review May 7, 2025 08:23
My idea was to have from_dict independent of the class
to cope for future evolution, but if a new version of the
standard can't be implemented as a subclass of Pylock,
return type of from_dict would change, so it would be
a breaking change.

If/when a new version of the spec arrive, it will still be
time to design the API evolution, depending of the actual changes.
@sbidoul
Copy link
Member Author

sbidoul commented Jul 6, 2025

I consider this PR ready for review. I plan to add documentation after a first round of review, assuming the overall design is agreeable to packaging maintainers, of course.

@brettcannon
Copy link
Member

FYI I'm hoping to look at this post-EuroPython in hopes that it helps unblock pip so it can add installation support for pylock.toml.

@brettcannon brettcannon self-requested a review July 25, 2025 17:22
Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! Some docstring tweaks-- since that will be documentation --and potential test tweaks. You can feel free to ignore the assignment expression suggestions if you want.

Comment on lines 59 to 63
PYLOCK_FILE_NAME_RE = re.compile(r"^pylock\.([^.]+)\.toml$")


def is_valid_pylock_path(path: Path) -> bool:
return path.name == "pylock.toml" or bool(PYLOCK_FILE_NAME_RE.match(path.name))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the regex is not going to be private, we might as well make it work for all names.

Suggested change
PYLOCK_FILE_NAME_RE = re.compile(r"^pylock\.([^.]+)\.toml$")
def is_valid_pylock_path(path: Path) -> bool:
return path.name == "pylock.toml" or bool(PYLOCK_FILE_NAME_RE.match(path.name))
PYLOCK_FILE_NAME_RE = re.compile(r"^pylock\.(?:([^.]+)\.)toml$")
def is_valid_pylock_path(path: Path) -> bool:
return bool(PYLOCK_FILE_NAME_RE.match(path.name))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't planning to make the regex public, and it's currently not in __all__, so it's kind of private?.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefixed the regex and some other things with _.



def _toml_value(key: str, value: Any) -> Any:
if isinstance(value, (Version, Marker, SpecifierSet)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if isinstance(value, (Version, Marker, SpecifierSet)):
if isinstance(value, {Version, Marker, SpecifierSet}):

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to work, at least in Python 3.13.

}
with pytest.raises(PylockValidationError) as exc_info:
Pylock.from_dict(data)
assert str(exc_info.value) == "Invalid version: '2.x' in 'lock-version'"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better to just test that "2.x" is somewhere in the message.

Same goes for any of the other tests that's doing an exact exception message comparison instead of just testing the key details are somewhere in the string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure about that, since these also test that the message contains the correct context explaining where the error occurs.

run-on = 2025-03-06T12:28:57.760769
""" # noqa: E501
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth having a test to verify this example deserializes as expected?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say test_toml_roundtrip does it implicitly? Or do you have something else in mind?

@sbidoul sbidoul force-pushed the pylock branch 5 times, most recently from 44f8995 to 3bbb50d Compare July 27, 2025 09:16
sbidoul added 3 commits July 27, 2025 13:55
This is more consitent than having some
validations done in the constructor and other in from_dict.
@sbidoul
Copy link
Member Author

sbidoul commented Jul 27, 2025

@brettcannon thanks for the review. I handled the comments and added a documentation page. This is ready for a second round.


def validate(self) -> None:
"""Validate the Pylock instance against the specification."""
self.from_dict(self.to_dict())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: we may want to catch Exception out of to_dict first and re-raise as PylockValidationError. from_dict should only raise PylockValidationError already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standard APIs for PEP 751?
2 participants