Fix a version comparison of public version with local version zero by dimbleby · Pull Request #920 · python-poetry/poetry-core (original) (raw)

Please address the comments from this code review:

Overall Comments

• For AlwaysSmaller/AlwaysGreater the __hash__ implementations could be made clearer and more stable by using a constant (e.g. hash(AlwaysSmaller) / hash(AlwaysGreater)) instead of id(...), which is process-specific and reads as an implementation detail rather than intent.
• AlwaysGreater.__gt__ (and similarly AlwaysSmaller’s implied ordering) currently returns True for any other, including other instances of the same type; combined with the new equality semantics this means two equal sentinels can also compare as greater than each other, so consider tightening the comparison logic to avoid contradictory ordering (e.g. short-circuit when other is an instance of the same class).

Individual Comments

Comment 1

This currently covers only 1.0 vs 1.0+0. To more fully exercise the comparison logic, please also add cases such as:

• 1.0+0 vs 1.0+1 (ordering within local versions)
• 1.0+abc vs 1.0+abc (equality of identical locals)
• 1.0+abc vs 1.0+ABC, if case-insensitivity/normalization is relevant in your PEP 440 implementation. These can be additional tests or parameters to this one.

Comment 2

Since this change adds __eq__/__hash__ to the AlwaysSmaller/AlwaysGreater helpers used in version comparisons, please add tests that indirectly validate their equality and hashing behavior via PEP440Version. For example, use boundary versions that trigger these sentinels in sets/dicts and assert correct lookups and equality behavior. If you know specific versions that exercise the sentinels, targeted tests for those would be ideal.

Suggested implementation:

def test_local_version_ordering_and_equality() -> None:
    """PEP 440: versions with a local segment must sort after the same
    version without one, and they must not compare equal.
    """
    v_plain = PEP440Version.parse("1.0")
    v_local = PEP440Version.parse("1.0+0")

    # 1.0+0 must be greater than 1.0 (local versions sort after)
    assert v_local > v_plain
    # They must not be equal
    assert v_plain != v_local


def test_version_hash_and_set_behavior_for_equivalent_final_versions() -> None:
    """Versions that are equal according to PEP 440 must behave correctly in
    sets and dicts, indirectly exercising comparison sentinels' hashing.

    In particular, final releases without pre/post/dev/local segments rely on
    sentinel values in their comparison keys. Two different textual
    representations like "1.0" and "1.0.0" should still be equal and have
    identical hashes.
    """
    v_short = PEP440Version.parse("1.0")
    v_long = PEP440Version.parse("1.0.0")

    # Sanity: these should compare equal
    assert v_short == v_long
    assert not (v_short != v_long)

    # Hashes must match for equal versions (comparison key includes sentinels)
    assert hash(v_short) == hash(v_long)

    # Sets must treat them as a single element
    versions_set = {v_short, v_long}
    assert len(versions_set) == 1
    assert v_short in versions_set
    assert v_long in versions_set

    # Dicts must allow lookup using an equivalent version instance
    versions_dict = {v_short: "value"}
    assert versions_dict[v_long] == "value"


def test_version_hash_and_set_behavior_for_versions_without_pre_or_dev() -> None:
    """Versions that omit pre/dev segments use sentinel values for those
    positions in the comparison key. Two semantically equal versions that
    differ only by normalization should still behave correctly in sets/dicts.
    """
    v_plain = PEP440Version.parse("1.0.post1")
    # An equivalent normalized form that should compare equal if the parser
    # normalizes post releases accordingly.
    v_equiv = PEP440Version.parse("1.0-post1")

    # They must compare equal if the normalization treats these as the same
    # version; if not, this test will reveal the discrepancy.
    assert v_plain == v_equiv
    assert hash(v_plain) == hash(v_equiv)

    versions_set = {v_plain, v_equiv}
    assert len(versions_set) == 1
    assert v_plain in versions_set
    assert v_equiv in versions_set

    versions_dict = {v_plain: "post-release"}
    assert versions_dict[v_equiv] == "post-release"

If "1.0-post1" is not accepted or not normalized as equal to "1.0.post1" in your implementation, adjust the second test to use a pair of known-equal textual representations that rely on sentinel-backed components in the comparison key. For example, if available, another suitable pair could be different-but-equivalent spellings of the same pre-release (e.g. "1.0a1" vs "1.0a1.0") or any other representations that parse to equal PEP440Version instances while omitting some of the pre/post/dev/local components.