Skip to content
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
20 changes: 9 additions & 11 deletions .flake8
Original file line number Diff line number Diff line change
@@ -1,22 +1,20 @@
[flake8]
Copy link
Copy Markdown
Member

@neersighted neersighted Nov 15, 2021

Choose a reason for hiding this comment

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

This whole file is very cargo-culted. Can we clean it up while we're in here?

max-line-length = 88
ignore = E501, E203, W503
per-file-ignores = __init__.py:F401
ignore = E501, E203, W503, ANN101, ANN102
mypy-init-return = True
Comment thread
neersighted marked this conversation as resolved.
per-file-ignores =
__init__.py:F401
tests/test_*:ANN201
tests/**/test_*:ANN201
exclude =
.git
__pycache__
setup.py
build
dist
releases
.venv
.tox
.mypy_cache
.pytest_cache
.vscode
.github
poetry_core/_vendor/
poetry_core/utils/_compat.py
poetry_core/utils/_typing.py
tests/fixtures/
tests/masonry/fixtures/
poetry/core/_vendor/*
tests/fixtures/*
tests/**/fixtures/*
7 changes: 1 addition & 6 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,9 @@ repos:
hooks:
- id: flake8
additional_dependencies:
- flake8-annotations
- flake8-bugbear
- flake8-comprehensions
exclude: |
(?x)(
^poetry/core/utils/_typing.py$
| ^poetry/core/utils/_compat.py$
| ^poetry/core/_vendor
)

- repo: https://github.com/pre-commit/mirrors-mypy
rev: v0.910
Expand Down
2 changes: 1 addition & 1 deletion poetry/core/semver/version.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ def from_parts(
post: Optional[ReleaseTag] = None,
dev: Optional[ReleaseTag] = None,
local: "LocalSegmentType" = None,
):
) -> "Version":
return cls(
release=Release(major=major, minor=minor, patch=patch, extra=extra),
pre=pre,
Expand Down
4 changes: 2 additions & 2 deletions poetry/core/vcs/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ def __str__(self) -> str:
_executable: Optional[str] = None


def executable():
def executable() -> str:
global _executable

if _executable is not None:
Expand Down Expand Up @@ -220,7 +220,7 @@ def executable():
return _executable


def _reset_executable():
def _reset_executable() -> None:
global _executable

_executable = None
Expand Down
3 changes: 2 additions & 1 deletion poetry/core/version/parser.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from pathlib import Path
from typing import TYPE_CHECKING
from typing import Any
from typing import Optional


Expand All @@ -17,7 +18,7 @@ def __init__(
self._debug = debug
self._lark: Optional["Lark"] = None

def parse(self, text: str, **kwargs) -> "Tree":
def parse(self, text: str, **kwargs: Any) -> "Tree":
from lark import Lark

if self._lark is None:
Expand Down
4 changes: 3 additions & 1 deletion poetry/core/version/pep440/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ def _get_local(cls, match: Optional[Match[AnyStr]]) -> Optional[LocalSegmentType
)

@classmethod
def parse(cls, value: str, version_class: Optional[Type["PEP440Version"]] = None):
def parse(
cls, value: str, version_class: Optional[Type["PEP440Version"]] = None
) -> "PEP440Version":
Comment on lines +64 to +66
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if you remove the if TYPE_CHECKING guard, this can be written as-

Suggested change
def parse(
cls, value: str, version_class: Optional[Type["PEP440Version"]] = None
) -> "PEP440Version":
def parse(
cls, value: str, version_class: Optional[PEP440Version] = None
) -> PEP440Version:

match = cls._regex.search(value) if value else None
if not match:
raise InvalidVersion(f"Invalid PEP 440 version: '{value}'")
Expand Down
4 changes: 2 additions & 2 deletions poetry/core/version/pep440/segments.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class Release:
default=None, init=False, compare=True
)

def __post_init__(self):
def __post_init__(self) -> None:
if self.extra is None:
object.__setattr__(self, "extra", ())
elif not isinstance(self.extra, tuple):
Expand Down Expand Up @@ -110,7 +110,7 @@ class ReleaseTag:
phase: str
number: int = dataclasses.field(default=0)

def __post_init__(self):
def __post_init__(self) -> None:
object.__setattr__(self, "phase", self.expand(self.phase))

@classmethod
Expand Down
13 changes: 9 additions & 4 deletions poetry/core/version/pep440/version.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import dataclasses
import math

from typing import Any
from typing import Optional
from typing import Tuple
from typing import Union
Expand Down Expand Up @@ -32,7 +33,7 @@ class PEP440Version:
int, Release, ReleaseTag, ReleaseTag, ReleaseTag, Tuple[Union[int, str], ...]
] = dataclasses.field(default=None, init=False, compare=True)

def __post_init__(self):
def __post_init__(self) -> None:
if self.local is not None and not isinstance(self.local, tuple):
object.__setattr__(self, "local", (self.local,))

Expand All @@ -46,7 +47,11 @@ def __post_init__(self):

object.__setattr__(self, "_compare_key", self._make_compare_key())

def _make_compare_key(self):
def _make_compare_key(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wow, this is a complicated type. Is there any way to clean this up with NewType, a refactor with an object, or similar?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was happy, that PyCharm was doing this for me 😆 IMO refactoring should be done in another PR, because the purpose of this PR is to enforce type hints in the future.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That is a brutal type signature :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've tried to understand the code and IMO the signature can be written more easily. See code change.

self,
) -> Tuple[
int, Release, ReleaseTag, ReleaseTag, ReleaseTag, Tuple[Tuple[float, str], ...]
]:
"""
This code is based on the implementation of packaging.version._cmpkey(..)
"""
Expand Down Expand Up @@ -103,7 +108,7 @@ def patch(self) -> Optional[int]:
def non_semver_parts(self) -> Optional[Tuple[int]]:
return self.release.extra

def to_string(self, short=False):
def to_string(self, short: bool = False) -> str:
dash = "-" if not short else ""

version_string = dash.join(
Expand Down Expand Up @@ -209,7 +214,7 @@ def first_prerelease(self) -> "PEP440Version":
epoch=self.epoch, release=self.release, pre=ReleaseTag(RELEASE_PHASE_ALPHA)
)

def replace(self, **kwargs):
def replace(self, **kwargs: Any) -> "PEP440Version":
return self.__class__(
**{
**{
Expand Down
4 changes: 2 additions & 2 deletions stanza
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class VendorUpdateCommand(Command):
argument("packages", "The packages to vendor.", optional=True, multiple=True)
]

def handle(self):
def handle(self) -> None:
packages = self.argument("packages")
current_dir = os.getcwd()
base = os.path.dirname(__file__)
Expand Down Expand Up @@ -182,7 +182,7 @@ class VendorCommand(Command):

commands = [VendorUpdateCommand()]

def handle(self):
def handle(self) -> int:
return self.call("help", self.name)


Expand Down
36 changes: 21 additions & 15 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import sys

from pathlib import Path
from typing import TYPE_CHECKING
from typing import Callable

import pytest
Expand All @@ -10,7 +11,12 @@
from tests.testutils import tempfile


def pytest_addoption(parser):
if TYPE_CHECKING:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you shouldn't need the if TYPE_CHECKING pattern here- that's really only for preventing circular imports. You can just import these directly, and use them below without quoting the types

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's a convention I'm used to. If you need a module just for type hints, import it only in the if TYPE_CHECKING block.

It's easier to teach to other people ("Why are you sometime put import in that block and sometimes not?") and the overall chance that you have a circular import if you need this import just for type hint is quite big.

TL;DR: I prefer to leave it as it is.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fair enough. there's some interesting background on it here - larryhastings/co_annotations#1

in general, it seems that if TYPE_CHECKING is used iff

  • you would otherwise have a circular import
  • the import is 'expensive'

the Benevolent Dictator for Life seems to regard it is a painfully necessary hack.

that said, it's also clear from the thread that it's far from settled. And using it everywhere instead of the small number of cases where you need it (do you definitely need it anywhere in this codebase?) is at least consistent.

from _pytest.config import Config
from _pytest.config.argparsing import Parser


def pytest_addoption(parser: "Parser") -> None:
parser.addoption(
"--integration",
action="store_true",
Expand All @@ -20,15 +26,15 @@ def pytest_addoption(parser):
)


def pytest_configure(config):
def pytest_configure(config: "Config") -> None:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you can use Config unquoted if you remove the if TYPE_CHECKING pattern above

config.addinivalue_line("markers", "integration: mark integration tests")

if not config.option.integration:
config.option.markexpr = "not integration"


def get_project_from_dir(base_directory): # type: (Path) -> Callable[[str], Path]
def get(name): # type: (str) -> Path
def get_project_from_dir(base_directory: Path) -> Callable[[str], Path]:
def get(name: str) -> Path:
path = base_directory / name
if not path.exists():
raise FileNotFoundError(str(path))
Expand All @@ -38,45 +44,45 @@ def get(name): # type: (str) -> Path


@pytest.fixture(scope="session")
def project_source_root(): # type: () -> Path
def project_source_root() -> Path:
return Path(__file__).parent.parent


@pytest.fixture(scope="session")
def project_source_test_root(): # type: () -> Path
def project_source_test_root() -> Path:
return Path(__file__).parent


@pytest.fixture(scope="session")
def common_fixtures_directory(project_source_test_root): # type: (Path) -> Path
def common_fixtures_directory(project_source_test_root: Path) -> Path:
return project_source_test_root / "fixtures"


@pytest.fixture(scope="session")
def common_project(common_fixtures_directory): # type: (Path) -> Callable[[str], Path]
def common_project(common_fixtures_directory: Path) -> Callable[[str], Path]:
return get_project_from_dir(common_fixtures_directory)


@pytest.fixture(scope="session")
def masonry_fixtures_directory(project_source_test_root): # type: (Path) -> Path
def masonry_fixtures_directory(project_source_test_root: Path) -> Path:
return project_source_test_root / "masonry" / "builders" / "fixtures"


@pytest.fixture(scope="session")
def masonry_project(
masonry_fixtures_directory,
): # type: (Path) -> Callable[[str], Path]
masonry_fixtures_directory: Path,
) -> Callable[[str], Path]:
return get_project_from_dir(masonry_fixtures_directory)


@pytest.fixture
def temporary_directory(): # type: () -> Path
def temporary_directory() -> Path:
with tempfile.TemporaryDirectory(prefix="poetry-core") as tmp:
yield Path(tmp)


@pytest.fixture
def venv(temporary_directory): # type: (Path) -> Path
def venv(temporary_directory: Path) -> Path:
venv_dir = temporary_directory / ".venv"
virtualenv.cli_run(
[
Expand All @@ -91,10 +97,10 @@ def venv(temporary_directory): # type: (Path) -> Path


@pytest.fixture
def python(venv): # type: (Path) -> str
def python(venv: Path) -> str:
return (venv / "bin" / "python").as_posix()


@pytest.fixture()
def f(): # type: () -> Factory
def f() -> Factory:
return Factory()
18 changes: 12 additions & 6 deletions tests/integration/test_pep517.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from pathlib import Path
from typing import TYPE_CHECKING

import pytest

Expand All @@ -9,6 +10,9 @@
from tests.testutils import temporary_project_directory


if TYPE_CHECKING:
from _pytest.fixtures import FixtureRequest

pytestmark = pytest.mark.integration


Expand All @@ -20,32 +24,34 @@
("masonry_project", "disable_setup_py"),
],
)
def test_pep517_check_poetry_managed(request, getter, project):
def test_pep517_check_poetry_managed(
request: "FixtureRequest", getter: str, project: str
):
with temporary_project_directory(request.getfixturevalue(getter)(project)) as path:
assert check(path)


def test_pep517_check(project_source_root):
def test_pep517_check(project_source_root: Path):
assert check(str(project_source_root))


def test_pep517_build_sdist(temporary_directory, project_source_root):
def test_pep517_build_sdist(temporary_directory: Path, project_source_root: Path):
build(
source_dir=str(project_source_root), dist="sdist", dest=str(temporary_directory)
)
distributions = list(temporary_directory.glob("poetry-core-*.tar.gz"))
assert len(distributions) == 1


def test_pep517_build_wheel(temporary_directory, project_source_root):
def test_pep517_build_wheel(temporary_directory: Path, project_source_root: Path):
build(
source_dir=str(project_source_root), dist="wheel", dest=str(temporary_directory)
)
distributions = list(temporary_directory.glob("poetry_core-*-none-any.whl"))
assert len(distributions) == 1


def test_pip_wheel_build(temporary_directory, project_source_root):
def test_pip_wheel_build(temporary_directory: Path, project_source_root: Path):
tmp = str(temporary_directory)
pip = subprocess_run(
"pip", "wheel", "--use-pep517", "-w", tmp, str(project_source_root)
Expand All @@ -58,7 +64,7 @@ def test_pip_wheel_build(temporary_directory, project_source_root):
assert len(wheels) == 1


def test_pip_install_no_binary(python, project_source_root):
def test_pip_install_no_binary(python: str, project_source_root: Path):
subprocess_run(
python,
"-m",
Expand Down
Loading