Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #417 +/- ##
==========================================
+ Coverage 73.98% 74.40% +0.41%
==========================================
Files 61 61
Lines 6762 6692 -70
Branches 1197 1182 -15
==========================================
- Hits 5003 4979 -24
+ Misses 1296 1256 -40
+ Partials 463 457 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dweindl
force-pushed
the
refactor_tables
branch
2 times, most recently
from
e01dfe1 to
2ff200f
Compare
m-philipps
left a comment
m-philipps
left a comment
There was a problem hiding this comment.
Looks great, feel free to ignore the comments, which are probably more relevant at the cosmetic stage.
| ) | ||
| return self.__class__(elements=self.elements + [other]) | ||
|
|
||
| def __iadd__(self, other: T) -> BaseTable[T]: |
There was a problem hiding this comment.
Why return self if this is inplace?
There was a problem hiding this comment.
https://docs.python.org/3/reference/datamodel.html#object.__iadd__:
object.__iadd__[...] These methods should attempt to do the operation in-place (modifying self) and return the result
| ] | ||
|
|
||
| return cls(observables=observables) | ||
| return cls(observables) |
There was a problem hiding this comment.
This is using a positional arg instead of elements=observables unlike e.g. in the BaseTable class
There was a problem hiding this comment.
Yes, I don't see a problem there.
| self.mappings.append(other) | ||
| return self | ||
|
|
||
| def __getitem__(self, petab_id: str) -> Mapping: |
There was a problem hiding this comment.
Is this still needed with having the BaseModel function?
There was a problem hiding this comment.
Yes, because Mapping has no id attribute. There we currently select based on petab_id.
3 participants
DRY
Introduce
BaseTableto implement common functionality of{Observable,Parameter,...}Table.