Skip to content

Commit

Permalink
Replace named tuples with data classes
Browse files Browse the repository at this point in the history
Data classes are superior in every aspect except one, data classes
cannot be unpacked:

```python
stdout, _ = ShellScript(...).run()
```

But this is rarely used to for anything that could not be replaced with
a data class:

```python
stdout,_ = ShellScript(...).run()
self.info('foo', stdout)
```

As `stdout` exists only to be passed to another call, without any
computation, a data class without unpacking does not look like a downgrade:

```
output = ShellScript(...).run()
self.info('foo', output.stdout)
```

Data classes are often faster, can be made immutable, and are aligned
with the rest of tmt code base where named tuples are exception, while
data classes are very common.
  • Loading branch information
happz committed Jun 22, 2023
1 parent b90533c commit 197f0f7
Show file tree
Hide file tree
Showing 10 changed files with 46 additions and 44 deletions.
30 changes: 15 additions & 15 deletions tests/unit/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -547,59 +547,59 @@ def test_multiple_values(self):


def test_run_interactive_not_joined(tmpdir, root_logger):
stdout, stderr = ShellScript("echo abc; echo def >2").to_shell_command().run(
output = ShellScript("echo abc; echo def >2").to_shell_command().run(
shell=True,
interactive=True,
cwd=Path(str(tmpdir)),
env={},
log=None,
logger=root_logger)
assert stdout is None
assert stderr is None
assert output.stdout is None
assert output.stderr is None


def test_run_interactive_joined(tmpdir, root_logger):
stdout, _ = ShellScript("echo abc; echo def >2").to_shell_command().run(
output = ShellScript("echo abc; echo def >2").to_shell_command().run(
shell=True,
interactive=True,
cwd=Path(str(tmpdir)),
env={},
join=True,
log=None,
logger=root_logger)
assert stdout is None
assert output.stdout is None


def test_run_not_joined_stdout(root_logger):
stdout, stderr = Command("ls", "/").run(
output = Command("ls", "/").run(
shell=False,
cwd=Path.cwd(),
env={},
log=None,
logger=root_logger)
assert "sbin" in stdout
assert "sbin" in output.stdout


def test_run_not_joined_stderr(root_logger):
_, stderr = ShellScript("ls non_existing || true").to_shell_command().run(
output = ShellScript("ls non_existing || true").to_shell_command().run(
shell=False,
cwd=Path.cwd(),
env={},
log=None,
logger=root_logger)
assert "ls: cannot access" in stderr
assert "ls: cannot access" in output.stderr


def test_run_joined(root_logger):
stdout, _ = ShellScript("ls non_existing / || true").to_shell_command().run(
output = ShellScript("ls non_existing / || true").to_shell_command().run(
shell=False,
cwd=Path.cwd(),
env={},
log=None,
join=True,
logger=root_logger)
assert "ls: cannot access" in stdout
assert "sbin" in stdout
assert "ls: cannot access" in output.stdout
assert "sbin" in output.stdout


def test_run_big(root_logger):
Expand All @@ -612,15 +612,15 @@ def test_run_big(root_logger):
done
"""

stdout, _ = ShellScript(textwrap.dedent(script)).to_shell_command().run(
output = ShellScript(textwrap.dedent(script)).to_shell_command().run(
shell=False,
cwd=Path.cwd(),
env={},
log=None,
join=True,
logger=root_logger)
assert "n n" in stdout
assert len(stdout) == 200000
assert "n n" in output.stdout
assert len(output.stdout) == 200000


def test_get_distgit_handler():
Expand Down
6 changes: 4 additions & 2 deletions tmt/options.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
""" Common options and the MethodCommand class """

import contextlib
import dataclasses
import re
import textwrap
from typing import TYPE_CHECKING, Any, Callable, Dict, List, NamedTuple, Optional, Type, Union
from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional, Type, Union

import click

Expand All @@ -27,7 +28,8 @@
import tmt.utils


class Deprecated(NamedTuple):
@dataclasses.dataclass(frozen=True)
class Deprecated:
""" Version information and hint for obsolete options """
since: str
hint: Optional[str] = None
Expand Down
6 changes: 3 additions & 3 deletions tmt/steps/discover/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,13 @@ def extract_distgit_source(
Source tarball is discovered from the 'sources' file content.
"""
if handler_name is None:
stdout, _ = self.run(
output = self.run(
Command("git", "config", "--get-regexp", '^remote\\..*.url'),
cwd=distgit_dir)
if stdout is None:
if output.stdout is None:
raise tmt.utils.GeneralError("Missing remote origin url.")

remotes = stdout.split('\n')
remotes = output.stdout.split('\n')
handler = tmt.utils.get_distgit_handler(remotes=remotes)
else:
handler = tmt.utils.get_distgit_handler(usage_name=handler_name)
Expand Down
20 changes: 10 additions & 10 deletions tmt/steps/discover/fmf.py
Original file line number Diff line number Diff line change
Expand Up @@ -260,12 +260,12 @@ def go(self) -> None:
"the `--dist-git-merge` option.")

def get_git_root(dir: Path) -> Path:
stdout, _ = self.run(
output = self.run(
Command("git", "rev-parse", "--show-toplevel"),
cwd=dir,
ignore_dry=True)
assert stdout is not None
return Path(stdout.strip("\n"))
assert output.stdout is not None
return Path(output.stdout.strip("\n"))

# Raise an exception if --fmf-id uses w/o url and git root
# doesn't exist for discovered plan
Expand Down Expand Up @@ -384,10 +384,10 @@ def assert_git_url(plan_name: Optional[str] = None) -> None:
# Show current commit hash if inside a git repository
if self.testdir.is_dir():
try:
hash_, _ = self.run(Command("git", "rev-parse", "--short", "HEAD"),
cwd=self.testdir)
if hash_ is not None:
self.verbose('hash', hash_.strip(), 'green')
output = self.run(Command("git", "rev-parse", "--short", "HEAD"),
cwd=self.testdir)
if output.stdout is not None:
self.verbose('hash', output.stdout.strip(), 'green')
except (tmt.utils.RunError, AttributeError):
pass

Expand Down Expand Up @@ -507,9 +507,9 @@ def assert_git_url(plan_name: Optional[str] = None) -> None:
output = self.run(
Command(
'git', 'log', '--format=', '--stat', '--name-only', f"{modified_ref}..HEAD"
), cwd=self.testdir)[0]
if output:
directories = [os.path.dirname(name) for name in output.split('\n')]
), cwd=self.testdir)
if output.stdout:
directories = [os.path.dirname(name) for name in output.stdout.split('\n')]
modified = {f"^/{re.escape(name)}" for name in directories if name}
self.debug(f"Limit to modified test dirs: {modified}", level=3)
names.extend(modified)
Expand Down
6 changes: 3 additions & 3 deletions tmt/steps/discover/shell.py
Original file line number Diff line number Diff line change
Expand Up @@ -347,9 +347,9 @@ def go(self) -> None:
run_result = self.run(
Command("git", "rev-parse", "--show-toplevel"),
cwd=self.step.plan.my_run.tree.root,
ignore_dry=True)[0]
assert run_result is not None
git_root = Path(run_result.strip('\n'))
ignore_dry=True)
assert run_result.stdout is not None
git_root = Path(run_result.stdout.strip('\n'))
except tmt.utils.RunError:
assert self.step.plan.my_run is not None # narrow type
assert self.step.plan.my_run.tree is not None # narrow type
Expand Down
3 changes: 2 additions & 1 deletion tmt/steps/execute/internal.py
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ def _test_output_logger(
# Execute the test, save the output and return code
starttime = datetime.datetime.now(datetime.timezone.utc)
try:
stdout, _ = guest.execute(
output = guest.execute(
remote_command,
cwd=workdir,
env=environment,
Expand All @@ -267,6 +267,7 @@ def _test_output_logger(
test_session=True,
friendly_command=str(test.test))
test.returncode = 0
stdout = output.stdout
except tmt.utils.RunError as error:
stdout = error.stdout
test.returncode = error.returncode
Expand Down
4 changes: 2 additions & 2 deletions tmt/steps/provision/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -941,11 +941,11 @@ def ansible(self, playbook: Path, extra_args: Optional[str] = None) -> None:
# FIXME: cast() - https://github.com/teemtee/tmt/issues/1372
parent = cast(Provision, self.parent)

stdout, _ = self.run(
output = self.run(
ansible_command,
cwd=parent.plan.worktree,
env=self._prepare_environment())
self._ansible_summary(stdout)
self._ansible_summary(output.stdout)

@property
def is_ready(self) -> bool:
Expand Down
4 changes: 2 additions & 2 deletions tmt/steps/provision/local.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def is_ready(self) -> bool:
def ansible(self, playbook: Path, extra_args: Optional[str] = None) -> None:
""" Prepare localhost using ansible playbook """
playbook = self._ansible_playbook_path(playbook)
stdout, _ = self.run(
output = self.run(
Command(
'sudo', '-E',
'ansible-playbook',
Expand All @@ -39,7 +39,7 @@ def ansible(self, playbook: Path, extra_args: Optional[str] = None) -> None:
'-i', 'localhost,',
str(playbook)),
env=self._prepare_environment())
self._ansible_summary(stdout)
self._ansible_summary(output.stdout)

def execute(self,
command: Union[Command, ShellScript],
Expand Down
7 changes: 3 additions & 4 deletions tmt/steps/provision/podman.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ def is_ready(self) -> bool:
'--format', '{{json .State.Running}}',
self.container
))
cmd_stdout, cmd_stderr = cmd_output
return str(cmd_stdout).strip() == 'true'
return str(cmd_output.stdout).strip() == 'true'

def wake(self) -> None:
""" Wake up the guest """
Expand Down Expand Up @@ -141,11 +140,11 @@ def ansible(self, playbook: Path, extra_args: Optional[str] = None) -> None:
'-c', 'podman', '-i', f'{self.container},', str(playbook)
]

stdout, _ = self.run(
output = self.run(
podman_command,
cwd=self.parent.plan.worktree,
env=self._prepare_environment())
self._ansible_summary(stdout)
self._ansible_summary(output.stdout)

def podman(self, command: Command, **kwargs: Any) -> tmt.utils.CommandOutput:
""" Run given command via podman """
Expand Down
4 changes: 2 additions & 2 deletions tmt/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
Generic,
Iterable,
List,
NamedTuple,
Optional,
Pattern,
Sequence,
Expand Down Expand Up @@ -320,7 +319,8 @@ def get_output(self) -> Optional[str]:
_CommandElement = str


class CommandOutput(NamedTuple):
@dataclasses.dataclass(frozen=True)
class CommandOutput:
stdout: Optional[str]
stderr: Optional[str]

Expand Down

0 comments on commit 197f0f7

Please sign in to comment.