Skip to content

refactor!: Drop VersionInfo in favor of tuple #124

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

Merged
merged 3 commits into from
Oct 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
default_language_version:
python: python3.10
python: python3.11
exclude: LICENSE
repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.4.0
rev: v4.5.0
hooks:
- id: check-merge-conflict
- id: check-json
Expand All @@ -20,11 +20,11 @@ repos:
hooks:
- id: check-manifest
- repo: https://github.com/psf/black
rev: 23.7.0
rev: 23.9.1
hooks:
- id: black
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.0.283
rev: v0.1.0
hooks:
- id: ruff
args: [--fix, --exit-non-zero-on-fix, --show-fixes]
51 changes: 11 additions & 40 deletions graphql_server/version.py
Copy link
Member

Choose a reason for hiding this comment

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

Honestly the way we handle version strings in this library seems very 2.x-ish. I'm all for just removing the logic and replacing it with something along the lines of this:

from dataclasses import dataclass
from packaging.version import parse

__all__ = ["VersionInfo", "version", "version_info"]

version = "3.0.0b7"

@dataclass
class VersionInfo:
    major: int
    minor: int
    micro: int
    releaselevel: str
    serial: int

    @classmethod
    def from_str(cls, v: str) -> "VersionInfo":
        parsed_version = parse(v)
        return cls(
            major=parsed_version.major,
            minor=parsed_version.minor,
            micro=parsed_version.micro,
            releaselevel=parsed_version.pre[0] if parsed_version.pre else "final",
            serial=parsed_version.pre[1] if parsed_version.pre else 0
        )

    def __str__(self) -> str:
        if self.releaselevel == "candidate":
            return f"{self.major}.{self.minor}.{self.micro}rc{self.serial}"
        elif self.releaselevel != "final":
            return f"{self.major}.{self.minor}.{self.micro}{self.releaselevel[0]}{self.serial}"
        return f"{self.major}.{self.minor}.{self.micro}"

version_info = VersionInfo.from_str(version)

Or drop the entire VersionInfo altogether, since this is a feature well-supported in packaging now. Since we are doing a major release, this shouldn't be a problem. What do you think? 😊

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's what I thought too. Not a fan of maintaining our own semver parser and as you said all the logics are already implemented in packaging.

We could keep exposing graphql_server.version = v3.0.0 and graphql_server.version_info = (3, 0, 0, "final", 0) and keep tests using packaging to check if version and version_info are spec-compliant and pointing to the same version.

Copy link
Member

@erikwrede erikwrede Oct 16, 2023

Choose a reason for hiding this comment

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

Sounds good, let's just do the tuple one and completely drop the version info.

Original file line number Diff line number Diff line change
@@ -1,44 +1,15 @@
import re
from typing import NamedTuple

__all__ = ["version", "version_info"]


version = "3.0.0b7"

_re_version = re.compile(r"(\d+)\.(\d+)\.(\d+)(\D*)(\d*)")


class VersionInfo(NamedTuple):
major: int
minor: int
micro: int
releaselevel: str
serial: int

@classmethod
def from_str(cls, v: str) -> "VersionInfo":
groups = _re_version.match(v).groups() # type: ignore
major, minor, micro = map(int, groups[:3])
level = (groups[3] or "")[:1]
if level == "a":
level = "alpha"
elif level == "b":
level = "beta"
elif level in ("c", "r"):
level = "candidate"
else:
level = "final"
serial = groups[4]
serial = int(serial) if serial else 0
return cls(major, minor, micro, level, serial)

def __str__(self) -> str:
v = f"{self.major}.{self.minor}.{self.micro}"
level = self.releaselevel
if level and level != "final":
v = f"{v}{level[:1]}{self.serial}"
return v


version_info = VersionInfo.from_str(version)
version_info = (3, 0, 0, "beta", 7)
# version_info has the same format as django.VERSION
# https://github.com/django/django/blob/4a5048b036fd9e965515e31fdd70b0af72655cba/django/utils/version.py#L22
#
# examples
# "3.0.0" -> (3, 0, 0, "final", 0)
# "3.0.0rc1" -> (3, 0, 0, "rc", 1)
# "3.0.0b7" -> (3, 0, 0, "beta", 7)
# "3.0.0a2" -> (3, 0, 0, "alpha", 2)
#
# also see tests/test_version.py
1 change: 1 addition & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
"pytest-cov>=4,<5",
"Jinja2>=3.1,<4",
"sanic-testing>=22.3,<24",
"packaging==23.2",
]

dev_requires = [
Expand Down
96 changes: 34 additions & 62 deletions tests/test_version.py
Original file line number Diff line number Diff line change
@@ -1,78 +1,50 @@
import re
import packaging
from packaging.version import Version

import graphql_server
from graphql_server.version import VersionInfo, version, version_info
from graphql_server.version import version, version_info

_re_version = re.compile(r"(\d+)\.(\d+)\.(\d+)(?:([abc])(\d+))?$")
RELEASE_LEVEL = {"alpha": "a", "beta": "b", "rc": "rc", "final": None}


def test_create_version_info_from_fields():
v = VersionInfo(1, 2, 3, "alpha", 4)
assert v.major == 1
assert v.minor == 2
assert v.micro == 3
assert v.releaselevel == "alpha"
assert v.serial == 4
parsed_version = Version(version)


def test_create_version_info_from_str():
v = VersionInfo.from_str("1.2.3")
assert v.major == 1
assert v.minor == 2
assert v.micro == 3
assert v.releaselevel == "final"
assert v.serial == 0
v = VersionInfo.from_str("1.2.3a4")
assert v.major == 1
assert v.minor == 2
assert v.micro == 3
assert v.releaselevel == "alpha"
assert v.serial == 4
v = VersionInfo.from_str("1.2.3beta4")
assert v.major == 1
assert v.minor == 2
assert v.micro == 3
assert v.releaselevel == "beta"
assert v.serial == 4
v = VersionInfo.from_str("12.34.56rc789")
assert v.major == 12
assert v.minor == 34
assert v.micro == 56
assert v.releaselevel == "candidate"
assert v.serial == 789
def test_valid_version() -> None:
packaging.version.parse(version)


def test_serialize_as_str():
v = VersionInfo(1, 2, 3, "final", 0)
assert str(v) == "1.2.3"
v = VersionInfo(1, 2, 3, "alpha", 4)
assert str(v) == "1.2.3a4"
def test_valid_version_info() -> None:
"""version_info has to be a tuple[int, int, int, str, int]"""
assert isinstance(version_info, tuple)
assert len(version_info) == 5

major, minor, micro, release_level, serial = version_info
assert isinstance(major, int)
assert isinstance(minor, int)
assert isinstance(micro, int)
assert isinstance(release_level, str)
assert isinstance(serial, int)

def test_base_package_has_correct_version():
assert graphql_server.__version__ == version
assert graphql_server.version == version

def test_valid_version_release_level() -> None:
if parsed_version.pre is not None:
valid_release_levels = {v for v in RELEASE_LEVEL.values() if v is not None}
assert parsed_version.pre[0] in valid_release_levels

def test_base_package_has_correct_version_info():
assert graphql_server.__version_info__ is version_info
assert graphql_server.version_info is version_info

def test_valid_version_info_release_level() -> None:
assert version_info[3] in RELEASE_LEVEL.keys()

def test_version_has_correct_format():
assert isinstance(version, str)
assert _re_version.match(version)

def test_version_same_as_version_info() -> None:
assert (
parsed_version.major,
parsed_version.minor,
parsed_version.micro,
) == version_info[:3]

def test_version_info_has_correct_fields():
assert isinstance(version_info, tuple)
assert str(version_info) == version
groups = _re_version.match(version).groups() # type: ignore
assert version_info.major == int(groups[0])
assert version_info.minor == int(groups[1])
assert version_info.micro == int(groups[2])
if groups[3] is None: # pragma: no cover
assert groups[4] is None
else: # pragma: no cover
assert version_info.releaselevel[:1] == groups[3]
assert version_info.serial == int(groups[4])
release_level, serial = version_info[-2:]
if parsed_version.is_prerelease:
assert (RELEASE_LEVEL[release_level], serial) == parsed_version.pre
else:
assert (release_level, serial) == ("final", 0)