-
Notifications
You must be signed in to change notification settings - Fork 450
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
Remove redundant arguments from components using kw_only=True
#1495
Remove redundant arguments from components using kw_only=True
#1495
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1495 +/- ##
==========================================
+ Coverage 80.33% 80.61% +0.27%
==========================================
Files 95 104 +9
Lines 6602 7306 +704
==========================================
+ Hits 5304 5890 +586
- Misses 1298 1416 +118 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR
def __post_init__(self) -> None: | ||
# set version in `__post_init__` so that is | ||
# cannot be set in `__init__` and is always set | ||
self.version: t.Optional[int] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add self.type_id = self.__class__.__name__
in the post_init
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to discuss this -- can be done later. (The code is old, just moved around a bit, creating this diff.)
superduperdb/components/model.py
Outdated
artifact_attributes: t.ClassVar[t.Sequence[str]] = [ | ||
'object', | ||
'preprocess', | ||
'postprocess', | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid using mutable objects in such places
d28759e
to
8c0687e
Compare
8c0687e
to
edcec7e
Compare
Comments addressed, will merge. |
The current
Component
classes have very redundant and bloated signatures. This is due to thefact that we were trying to respect the ordering which
@dataclass
forces. By addingthe
@dataclass(kw_only=True)
decorator, we are able to avoid this mess, allowing for a significantcleaning up of signatures.
In the process of doing this, it was necessary to finally deal with the fact that
Component.version
should neverbe set via
__init__
.