Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #193 +/- ##
===========================================
- Coverage 81.15% 79.40% -1.76%
===========================================
Files 52 53 +1
Lines 4267 4403 +136
Branches 740 780 +40
===========================================
+ Hits 3463 3496 +33
- Misses 624 727 +103
Partials 180 180
Flags with carried forward coverage won't be shown. Click here to find out more.
|
rozyczko
left a comment
There was a problem hiding this comment.
-
I understand that the lack of graph integration is a feature? We let the list components do the edge/vertex tracking?
-
Good type safety but not sure we should allow duplicates. My ModelCollection just skips duplicates which might be even worse.
-
Where is the
dataproperty? Do we need it? -
Where is the aggregate
get_all_variables? Just run over all children and return that aggregate.
| return any(r.unique_name == item for r in self._data) | ||
| return item in self._data | ||
|
|
||
| def index(self, value: ProtectedType_ | str, start: int = 0, stop: int = ...) -> int: |
There was a problem hiding this comment.
I think this will throw TypeError in min(stop...) if stop is left as ... (ellipsis can't be compared to int)
You might need to change it to stop: int | None = None instead
|
I can find no problems with the code, but it's also using some methods that are new to me, so I'm not an expert. As discussed elsewhere, I'll want this exact class, but using something like From what I understand, we could make that somewhat straightforward by introducing something like and replace calls to This seems to me to be cleaner than maintaining a copy of EasyList in EasyDynamics. I approve the name BTW. |
Here is my initial thoughts on how the generic EasyList could look like. I haven't added tests yet, but please have a look.