Skip to content

Commit

Permalink
clean up env.run(..., call=True)
Browse files Browse the repository at this point in the history
- have it return a string, simplifying the type annotations
- have it check for error
  • Loading branch information
dimbleby committed Apr 19, 2023
1 parent a4c9535 commit 9945c7b
Show file tree
Hide file tree
Showing 5 changed files with 22 additions and 20 deletions.
6 changes: 3 additions & 3 deletions src/poetry/installation/pip_installer.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ def remove(self, package: Package) -> None:
if src_dir.exists():
remove_directory(src_dir, force=True)

def run(self, *args: Any, **kwargs: Any) -> int | str:
def run(self, *args: Any, **kwargs: Any) -> str:
return self._env.run_pip(*args, **kwargs)

def requirement(self, package: Package, formatted: bool = False) -> str | list[str]:
Expand Down Expand Up @@ -207,7 +207,7 @@ def create_temporary_requirement(self, package: Package) -> str:

return name

def install_directory(self, package: Package) -> str | int:
def install_directory(self, package: Package) -> str:
from cleo.io.null_io import NullIO

from poetry.factory import Factory
Expand Down Expand Up @@ -245,7 +245,7 @@ def install_directory(self, package: Package) -> str | int:
builder = EditableBuilder(package_poetry, self._env, NullIO())
builder.build()

return 0
return ""
elif legacy_pip or package_poetry.package.build_script:
from poetry.core.masonry.builders.sdist import SdistBuilder

Expand Down
19 changes: 10 additions & 9 deletions src/poetry/utils/env.py
Original file line number Diff line number Diff line change
Expand Up @@ -1499,16 +1499,16 @@ def get_command_from_bin(self, bin: str) -> list[str]:

return [str(self._bin(bin))]

def run(self, bin: str, *args: str, **kwargs: Any) -> str | int:
def run(self, bin: str, *args: str, **kwargs: Any) -> str:
cmd = self.get_command_from_bin(bin) + list(args)
return self._run(cmd, **kwargs)

def run_pip(self, *args: str, **kwargs: Any) -> int | str:
def run_pip(self, *args: str, **kwargs: Any) -> str:
pip = self.get_pip_command()
cmd = pip + list(args)
return self._run(cmd, **kwargs)

def run_python_script(self, content: str, **kwargs: Any) -> int | str:
def run_python_script(self, content: str, **kwargs: Any) -> str:
return self.run(
self._executable,
"-I",
Expand All @@ -1520,7 +1520,7 @@ def run_python_script(self, content: str, **kwargs: Any) -> int | str:
**kwargs,
)

def _run(self, cmd: list[str], **kwargs: Any) -> int | str:
def _run(self, cmd: list[str], **kwargs: Any) -> str:
"""
Run a command inside the Python environment.
"""
Expand All @@ -1542,7 +1542,8 @@ def _run(self, cmd: list[str], **kwargs: Any) -> int | str:
).stdout
elif call:
assert stderr != subprocess.PIPE
return subprocess.call(cmd, stderr=stderr, env=env, **kwargs)
subprocess.check_call(cmd, stderr=stderr, env=env, **kwargs)
output = ""
else:
output = subprocess.check_output(cmd, stderr=stderr, env=env, **kwargs)
except CalledProcessError as e:
Expand Down Expand Up @@ -1780,7 +1781,7 @@ def is_sane(self) -> bool:
# A virtualenv is considered sane if "python" exists.
return os.path.exists(self.python)

def _run(self, cmd: list[str], **kwargs: Any) -> int | str:
def _run(self, cmd: list[str], **kwargs: Any) -> str:
kwargs["env"] = self.get_temp_environ(environ=kwargs.get("env"))
return super()._run(cmd, **kwargs)

Expand Down Expand Up @@ -1902,7 +1903,7 @@ def execute(self, bin: str, *args: str, **kwargs: Any) -> int:

return exe.returncode

def _run(self, cmd: list[str], **kwargs: Any) -> int | str:
def _run(self, cmd: list[str], **kwargs: Any) -> str:
return super(VirtualEnv, self)._run(cmd, **kwargs)

def is_venv(self) -> bool:
Expand Down Expand Up @@ -1932,12 +1933,12 @@ def paths(self) -> dict[str, str]:

return self._paths

def _run(self, cmd: list[str], **kwargs: Any) -> int | str:
def _run(self, cmd: list[str], **kwargs: Any) -> str:
self.executed.append(cmd)

if self._execute:
return super()._run(cmd, **kwargs)
return 0
return ""

def execute(self, bin: str, *args: str, **kwargs: Any) -> int:
self.executed.append([bin] + list(args))
Expand Down
2 changes: 1 addition & 1 deletion src/poetry/utils/pip.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def pip_install(
editable: bool = False,
deps: bool = False,
upgrade: bool = False,
) -> int | str:
) -> str:
is_wheel = path.suffix == ".whl"

# We disable version check here as we are already pinning to version available in
Expand Down
2 changes: 1 addition & 1 deletion tests/puzzle/test_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@


class MockEnv(BaseMockEnv):
def run(self, bin: str, *args: str, **kwargs: Any) -> str | int:
def run(self, bin: str, *args: str, **kwargs: Any) -> str:
raise EnvCommandError(CalledProcessError(1, "python", output=""))


Expand Down
13 changes: 7 additions & 6 deletions tests/utils/test_env.py
Original file line number Diff line number Diff line change
Expand Up @@ -966,11 +966,11 @@ def test_call_with_input_and_keyboard_interrupt(
def test_call_no_input_with_keyboard_interrupt(
tmp_path: Path, tmp_venv: VirtualEnv, mocker: MockerFixture
) -> None:
mocker.patch("subprocess.call", side_effect=KeyboardInterrupt())
mocker.patch("subprocess.check_call", side_effect=KeyboardInterrupt())
kwargs = {"call": True}
with pytest.raises(KeyboardInterrupt):
tmp_venv.run("python", "-", **kwargs)
subprocess.call.assert_called_once()
subprocess.check_call.assert_called_once()


def test_run_with_called_process_error(
Expand Down Expand Up @@ -1010,15 +1010,15 @@ def test_call_no_input_with_called_process_error(
tmp_path: Path, tmp_venv: VirtualEnv, mocker: MockerFixture
) -> None:
mocker.patch(
"subprocess.call",
"subprocess.check_call",
side_effect=subprocess.CalledProcessError(
42, "some_command", "some output", "some error"
),
)
kwargs = {"call": True}
with pytest.raises(EnvCommandError) as error:
tmp_venv.run("python", "-", **kwargs)
subprocess.call.assert_called_once()
subprocess.check_call.assert_called_once()
assert "some output" in str(error.value)
assert "some error" in str(error.value)

Expand Down Expand Up @@ -1054,9 +1054,10 @@ def test_call_does_not_block_on_full_pipe(
)

def target(result: list[int]) -> None:
result.append(tmp_venv.run("python", str(script), call=True))
tmp_venv.run("python", str(script), call=True)
result.append(0)

results = []
results: list[int] = []
# use a separate thread, so that the test does not block in case of error
thread = Thread(target=target, args=(results,))
thread.start()
Expand Down

0 comments on commit 9945c7b

Please sign in to comment.