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

Use attrs #2831

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Use attrs #2831

wants to merge 3 commits into from

Conversation

LecrisUT
Copy link
Contributor

@LecrisUT LecrisUT commented Apr 5, 2024

Testing out how hard it would be to replace dataclass with attrs

  • Replace standalone dataclass: Trivial
  • attrs.define subclassing dataclasses.dataclass:
    • __init__ kwargs are not inheritted
  • Converting tmt.utils.field: unknown
    • How to simplify type-checking and forward all kwargs to click/metadata/attrs.field
  • Converting tmt.utils.FieldMetadata: unknown
  • Converting tmt.utils.DataContainer: unknown

Pull Request Checklist

  • implement the feature
  • write the documentation
  • extend the test coverage
  • update the specification
  • adjust plugin docstring
  • modify the json schema
  • mention the version
  • include a release note

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@martinhoyer
Copy link
Collaborator

Hi @LecrisUT, why attrs? Did I miss something?

tmt/hardware.py Outdated
@@ -72,7 +73,6 @@
# The default formatting should use unit symbols rather than full names.
UNITS.default_format = '~'


Copy link
Collaborator

Choose a reason for hiding this comment

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

One immediate comment, there are way too many formatting changes, obscuring the actual changes. Empty lines added and removed, indentation changes... I wonder how that happens, we should be using the same autopep8 settings in pre-commit.

The rest seems safely trivial, but need to re-read once the dust settles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, that must have been from my IDE, but I thought it was already setup to use ruff-format, so I'm not sure why this got changed and not in the pre-commit, I can review the commit more and minimize these

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Apr 5, 2024

@martinhoyer attrs has more convenient api for adding converter, validator, etc. We did discuss it a bit here and there, e.g. #2766. Overall it would be good to touch up on how dataclasses are used and see how this can be improved, simplified and what else we can gain from attrs. So far what this could help with:

  • Decoupling the dataclass from the python version
  • Simplifying the __init__ and such by having the normalizer and such handled by the attrs built-ins
  • Cleaner interface using decorators for validator and default (no decorator sintax for converter yet)
  • More extensibility potential. E.g. coupling with a json validator

@lukaszachy
Copy link
Collaborator

Just to note version: Oldest attrs we need to support is 20.3.0 in el9 (from appstream repo)

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Copy link
Contributor Author

I've tried something very basic like:

@attrs.define
class DiscoverCoprData(DiscoverStepData):
    packages: list[str] = tmt.utils.field(
        option=("-p", "--package"),
        default_factory=list,
    )

But this didn't go as intended, the __init__ did not inherit the kwargs

@LecrisUT LecrisUT changed the title feat: Use attrs Use attrs Apr 15, 2024
@LecrisUT LecrisUT changed the title Use attrs Use attrs Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants