Skip to content
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

Replace named tuples with data classes #2053

Merged
merged 1 commit into from
Jun 29, 2023
Merged

Conversation

happz
Copy link
Collaborator

@happz happz commented May 5, 2023

Data classes are superior in every aspect except one, data classes cannot be unpacked:

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

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

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.

@happz happz added this to the 1.24 milestone May 5, 2023
@happz happz force-pushed the dataclass-instead-of-namedtuple branch 2 times, most recently from 498fd2a to a54a068 Compare May 16, 2023 09:06
@happz happz added the no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. label May 22, 2023
@happz happz force-pushed the dataclass-instead-of-namedtuple branch from a54a068 to 532b5a5 Compare May 26, 2023 13:04
@happz happz modified the milestones: 1.24, 1.25 Jun 5, 2023
@happz happz force-pushed the dataclass-instead-of-namedtuple branch from 532b5a5 to 10a82e3 Compare June 21, 2023 07:20
@happz
Copy link
Collaborator Author

happz commented Jun 21, 2023

Downstream patch has been submitted.

@happz happz force-pushed the dataclass-instead-of-namedtuple branch from 10a82e3 to 197f0f7 Compare June 22, 2023 11:05
@psss psss self-assigned this Jun 29, 2023
Copy link
Collaborator

@psss psss left a comment

Choose a reason for hiding this comment

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

Nice improvement, thanks!

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.
@psss psss force-pushed the dataclass-instead-of-namedtuple branch from 197f0f7 to ccfb0d0 Compare June 29, 2023 15:25
@psss psss merged commit ccfb0d0 into main Jun 29, 2023
16 checks passed
@psss psss deleted the dataclass-instead-of-namedtuple branch June 29, 2023 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code style no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants