Remove long list of if/elif, use dict #48
Conversation
src/andromede/expression/visitor.py
Outdated
| TYPES = { | ||
| LiteralNode: "literal", | ||
| NegationNode: "negation", | ||
| VariableNode: "variable", | ||
| ParameterNode: "parameter", | ||
| ComponentParameterNode: "comp_parameter", | ||
| ComponentVariableNode: "comp_variable", | ||
| AdditionNode: "addition", | ||
| MultiplicationNode: "multiplication", | ||
| DivisionNode: "division", | ||
| SubstractionNode: "substraction", | ||
| ComparisonNode: "comparison", | ||
| TimeOperatorNode: "time_operator", | ||
| TimeAggregatorNode: "time_aggregator", | ||
| ScenarioOperatorNode: "scenario_operator", | ||
| PortFieldNode: "port_field", | ||
| PortFieldAggregatorNode: "port_field_aggregator", | ||
| } |
There was a problem hiding this comment.
It would be better to define this object once and for all, and not evaluate it for each function call. However I don't remember how to achieve this in Python.
There was a problem hiding this comment.
Maybe using a single dispatch from functools ?
https://docs.python.org/3/library/functools.html#functools.singledispatch
There was a problem hiding this comment.
Anyway, you already did what they proposed here:
https://docs.python.org/3/faq/programming.html#how-do-i-use-strings-to-call-functions-methods
You could change the key from a string to the function itself (or a lambda), maybe ?
tbittar
force-pushed
the
fix/remove-if-elif
branch
from
bbd79fb to
2cbe4d8
Compare
|
@sylvlecl should we merge this ? |
| elif isinstance(root, PortFieldAggregatorNode): | ||
| return visitor.port_field_aggregator(root) | ||
| raise ValueError(f"Unknown expression node type {root.__class__}") | ||
| TYPES = { |
There was a problem hiding this comment.
Not sure of the added value:
it weakens the type checking and possibilities of refactoring, without bringing much clarity or brevity.
Would be better with a mapping to the actual method, probably:
methods = {
LiteralNode: visitor.literal,
NegationNode: visitor.negation,
...
}
return methods[type(root)](root)We could also use a match in python 3.10+
There was a problem hiding this comment.
Mypy is not happy with methods[type(root)](root) as it does not know the type of the function it calls
|
After reviews of the different proposals in this PR, it seems that the original code is in fact the best for now |
No description provided.