Relax type for structurelist argument to TrajectoryData - #7536
Relax type for structurelist argument to TrajectoryData#7536danielhollas wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesTrajectory structure sequence support
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aiida/orm/nodes/data/array/trajectory.py (1)
221-226: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the
set_structurelistdocstring for sequence input.
set_structurelistnow acceptst.Sequence[StructureData], but the summary and:param structurelist:entry still describe a list. Change both descriptions to “sequence” and mention tuple support. Otherwise, the generated documentation contradicts the public type contract.As per coding guidelines, keep the accepted type in the annotation and describe the parameter consistently in the Sphinx prose.
Proposed documentation update
- """Create trajectory from the list of + """Create trajectory from a sequence of :py:class:`aiida.orm.nodes.data.structure.StructureData` instances. ... - :param structurelist: a list of + :param structurelist: a sequence of :py:class:`aiida.orm.nodes.data.structure.StructureData` instances.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiida/orm/nodes/data/array/trajectory.py` around lines 221 - 226, Update the set_structurelist docstring summary and its :param structurelist: description to say “sequence” instead of “list,” explicitly noting that tuples are supported, while preserving the existing t.Sequence[StructureData] annotation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/aiida/orm/nodes/data/array/trajectory.py`:
- Around line 221-226: Update the set_structurelist docstring summary and its
:param structurelist: description to say “sequence” instead of “list,”
explicitly noting that tuples are supported, while preserving the existing
t.Sequence[StructureData] annotation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 968be460-6349-4df9-82ce-91233be57361
📒 Files selected for processing (1)
src/aiida/orm/nodes/data/array/trajectory.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7536 +/- ##
==========================================
+ Coverage 80.68% 80.69% +0.02%
==========================================
Files 581 581
Lines 47068 47068
==========================================
+ Hits 37972 37977 +5
+ Misses 9096 9091 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Found when trying to typecheck my AiiDAlab application, where I am passing a
tuple[StructureData]as the structurelist argument toTrajectoryData