Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where calling len() on empty ListNode instances would raise an error instead of returning the correct length. The fix also includes refactoring of existing tests into a more comprehensive parameterized test structure.
- Simplified the
__len__method in CollectionNode to always return the length from node info - Moved the
getmethod from CollectionNode to DictNode where it belongs - Renamed
import_method toloadwith enhanced functionality for loading specific nodes from other DAGs
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/daggerml/core.py | Fixed __len__ method bug, moved get method to DictNode, renamed and enhanced import_ to load |
| tests/test_core.py | Added comprehensive parameterized tests and reorganized existing test methods |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| If default is not given, it defaults to None, so that this method never raises a KeyError. | ||
| """ | ||
| return make_node(self.dag, self.dag.dml.get(self, key, default, name=name, doc=doc)) | ||
| return self._info["length"] |
There was a problem hiding this comment.
The fix removes the conditional check for self._info["length"] but doesn't handle the case where the length might be None or missing. This could cause a KeyError or return None unexpectedly when len() is called on nodes that don't have length information.
It would raise an error before, and now it doesn't.
added a parameterization to the tests to check.