Skip to content

Add initial EasyList class#193

Open
damskii9992 wants to merge 1 commit intodevelopfrom
easylist2
Open

Add initial EasyList class#193
damskii9992 wants to merge 1 commit intodevelopfrom
easylist2

Conversation

@damskii9992
Copy link
Contributor

Here is my initial thoughts on how the generic EasyList could look like. I haven't added tests yet, but please have a look.

@damskii9992 damskii9992 added [scope] enhancement Adds/improves features (major.MINOR.patch) [priority] high Should be prioritized soon [area] base classes Changes to or creation of new base classes labels Feb 5, 2026
@codecov
Copy link

codecov bot commented Feb 5, 2026

Codecov Report

❌ Patch coverage is 24.26471% with 103 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.40%. Comparing base (bd10653) to head (3306c3c).

Files with missing lines Patch % Lines
src/easyscience/base_classes/easy_list.py 23.70% 103 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             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              
Flag Coverage Δ
unittests 79.40% <24.26%> (-1.76%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/easyscience/base_classes/__init__.py 100.00% <100.00%> (ø)
src/easyscience/base_classes/easy_list.py 23.70% <23.70%> (ø)

Copy link
Member

@rozyczko rozyczko left a comment

Choose a reason for hiding this comment

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

  • 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 data property? 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:
Copy link
Member

Choose a reason for hiding this comment

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

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

@henrikjacobsenfys
Copy link
Member

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 name instead of unique_name.

From what I understand, we could make that somewhat straightforward by introducing something like

def _get_key(self, item: ProtectedType_) -> str:
    return item.unique_name

and replace calls to item.unique_name with self._get_key(item).
That way I can inherit from this class and override _get_key (and write my own validation to ensure the name is unique within the specific list).

This seems to me to be cleaner than maintaining a copy of EasyList in EasyDynamics.

I approve the name BTW.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[area] base classes Changes to or creation of new base classes [priority] high Should be prioritized soon [scope] enhancement Adds/improves features (major.MINOR.patch)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants