Skip to content

Conversation

@FBumann
Copy link
Member

@FBumann FBumann commented Oct 19, 2025

Description

Brief description of the changes in this PR.

Type of Change

  • Bug fix
  • New feature
  • Documentation update
  • Code refactoring

Related Issues

Closes #393

Testing

  • I have tested my changes
  • Existing tests still pass

Checklist

  • My code follows the project style
  • I have updated documentation if needed
  • I have added tests for new functionality (if applicable)

Summary by CodeRabbit

  • Refactor
    • Added public, typed model/submodel attributes across core classes to make model references discoverable.
    • Standardized initialization of these attributes to None.
    • No behavioral changes; improves API visibility, editor/type-checker support, and IDE autocompletion.

@FBumann FBumann linked an issue Oct 19, 2025 that may be closed by this pull request
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 19, 2025

Walkthrough

Added public type-annotated model/submodel attributes across core classes (Calculation, FlowSystem, CalculationResults, components, elements, effects, and structure) and changed per-instance annotated initializations to plain assignments (self.xxx = None) where applicable. No runtime behavior or control-flow changes.

Changes

Cohort / File(s) Summary
High-level model attributes
flixopt/calculation.py, flixopt/flow_system.py, flixopt/results.py
Added public type-annotated model attributes: `Calculation.model: FlowSystemModel
Component submodel attributes
flixopt/components.py
Added public submodel annotations to component classes: `LinearConverter.submodel: LinearConverterModel
Element and structure submodel attributes
flixopt/elements.py, flixopt/structure.py
Added submodel annotations: `Bus.submodel: BusModel
Effect submodel attributes
flixopt/effects.py
Added submodel annotations: `Effect.submodel: EffectModel
Changelog
CHANGELOG.md
Documented development note about added type hints for model/submodel on interface classes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Feature/v3/update 2 #355 — Overlaps changes to FlowSystem and model/submodel usage; likely touches the same public attributes.
  • Feature/v3/data converter #356 — Related edits to DataConverter and class annotations that interact with model handling.
  • V3.0.0/main #284 — Earlier FlowSystemModel/Submodel refactor that introduced the patterns these annotations formalize.

Suggested labels

v3.0.0

Poem

🐇 I hop through code with a soft little hum,
Stashing model names where the types should come.
Submodels tucked in, all neat and aligned,
A carrot for clarity — the interfaces signed. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description follows the repository template structure but is incomplete in critical areas. The "Description" section contains only the placeholder text "Brief description of the changes in this PR." without actual details about what was changed or why. The "Type of Change" checkboxes are all unchecked, though "Code refactoring" would be appropriate for this PR. The "Related Issues" section is properly filled with "Closes #393," and the testing checkboxes remain unchecked with no explanation provided. The checklist items are marked as completed, but the lack of a substantive description section significantly diminishes the quality and clarity of the pull request documentation.
Docstring Coverage ⚠️ Warning Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Added type hint to all submodel attributes" is concise and directly related to the main changes in the pull request. The PR adds type annotations to model and submodel attributes across multiple files (Calculation, FlowSystem, components, effects, elements, results, and structure). While the title specifically mentions "submodel attributes" and the changes also include type hints for attributes named "model" (in Calculation, FlowSystem, and CalculationResults), this distinction represents an acceptable level of abstraction for a pull request title—the core intent of adding type hints to model/submodel attributes is clearly conveyed.
Linked Issues Check ✅ Passed The linked issue #393 requests adding type hints for model types to all interfaces, and the pull request implements this objective comprehensively. Changes include adding type annotations to model/submodel attributes across seven files: Calculation, FlowSystem, CalculationResults, three component classes (LinearConverter, Storage, Transmission), two effect classes (Effect, EffectCollection), two element classes (Bus, Flow), and the Element base class. Each annotation includes both the type and None as a union option (or just the specific type where appropriate), aligning with the goal of adding proper type hints to model-related attributes throughout the codebase.
Out of Scope Changes Check ✅ Passed All changes in the pull request are directly related to the stated objective of adding type hints to model/submodel attributes. The modifications consist entirely of adding type annotations to class attributes across multiple files and a corresponding CHANGELOG.md entry documenting the change. No behavioral modifications, new logic, or control flow changes are introduced—all alterations are purely structural annotations to improve type safety and IDE support. The changes are focused and consistent with the linked issue requirements.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 393-add-type-hints-of-model-type-to-all-interfaces

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4f3967d and deb9390.

📒 Files selected for processing (6)
  • CHANGELOG.md (1 hunks)
  • flixopt/components.py (3 hunks)
  • flixopt/effects.py (2 hunks)
  • flixopt/elements.py (2 hunks)
  • flixopt/results.py (1 hunks)
  • flixopt/structure.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • flixopt/results.py
  • flixopt/elements.py
🔇 Additional comments (5)
flixopt/structure.py (1)

859-869: LGTM! Clean type hint addition following best practices.

The class-level type annotation submodel: ElementModel | None combined with the None initialization in __init__ correctly establishes the pattern for all Element subclasses. This allows submodel to be None from construction until create_model() is called, which matches the actual lifecycle.

flixopt/effects.py (2)

162-163: LGTM! Type hint correctly added.

The submodel: EffectModel | None annotation properly narrows the parent Element's submodel type while maintaining the optional nature, as noted in the resolved past review comment.


458-465: LGTM! Consistent pattern applied.

The type annotation and initialization follow the same pattern as Element and Effect, correctly handling the lifecycle where submodel is None until create_model() assigns the EffectCollectionModel instance.

flixopt/components.py (1)

163-164: LGTM! Type hints correctly applied across all component classes.

All three component classes (LinearConverter, Storage, Transmission) now have properly typed submodel attributes with | None to reflect that they're initialized to None and only assigned in their respective create_model() methods (lines 181, 433, 711). This addresses the past review feedback and maintains consistency with the parent Element class pattern.

Also applies to: 381-382, 647-648

CHANGELOG.md (1)

74-74: LGTM! Changelog entry accurately documents the changes.

The development note correctly summarizes the type hint additions for submodel attributes across the Interface classes. Appropriately categorized under Development since this is an internal improvement without runtime behavior changes.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9b498e7 and d607f4b.

📒 Files selected for processing (7)
  • flixopt/calculation.py (2 hunks)
  • flixopt/components.py (3 hunks)
  • flixopt/effects.py (2 hunks)
  • flixopt/elements.py (2 hunks)
  • flixopt/flow_system.py (1 hunks)
  • flixopt/results.py (1 hunks)
  • flixopt/structure.py (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
flixopt/calculation.py (1)
flixopt/structure.py (1)
  • FlowSystemModel (82-253)
flixopt/flow_system.py (1)
flixopt/structure.py (1)
  • FlowSystemModel (82-253)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: test (3.13)
  • GitHub Check: test (3.11)
  • GitHub Check: test (3.12)
  • GitHub Check: test (3.10)

@FBumann
Copy link
Member Author

FBumann commented Oct 20, 2025

Also add hints for Dubmodle classes (which Interface)?

@FBumann FBumann merged commit b91f804 into main Oct 30, 2025
12 checks passed
@FBumann FBumann deleted the 393-add-type-hints-of-model-type-to-all-interfaces branch October 30, 2025 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add type hints of Model type to all Interfaces

2 participants