potassco / potassco/constraint-handler
Architecture Hierarchy and Potential Circular Imports.
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 3
- Forks
- 0
- Avg merge
- 1d 19h
- Merged PRs (30d)
- 20
Description
While working on making bad more fine-grained, I encountered an issue with circular imports.
Context
Currently, each module responsible for the evaluation of some type creates a class Evaluator and an enum Operator. This creates a clean-looking interface that can be used like so:
self.arithmetic = arithmetic.Evaluator(Evaluator, self.errors)
self.logic = logic.Evaluator(Evaluator, self.errors)
self.multimap = multimap.Evaluator(Evaluator, self.errors)
self.set = myset.Evaluator(Evaluator, self.errors)
However, there also exists schemas/ where schemas are defined that relate to some concept. In there, lies expression.py which also defines operators. Additionally, it defines Operator that contains all the other operators via:
Operator = (
arithmetic.Operator
| EqOperator
| logic.Operator
| StringOperator
| multimap.Operator
| myset.Operator
| OtherOperator
| ConditionalOperator
| Python
)
This requires the import of the respective sub-modules such as arithmetic,logic or multimap. And also creates Bad by importing it from common.
Issue
The role of the schemas/expression module seems to be the common interface between the different evaluation modules (evaluator, propagator). A module that provides types whenever expressions are concerned.
However, I think the architectural approach is sometimes unclear and seems inconsistent, which may lead to confusion.
For example, multimap defines the type HashableDict, but this type is not found in any schema. This leads the propagator variables to import it directly, now directly depending on the multimap module.
Additionally, Bad is now introduced in many places and ultimately defined in utils/common.py. But why is Bad a utility while EqOperator is not? Because we can't defined Bad inside the expression schema directly! Otherwise, we would get circular imports.
For example, when trying to make the isin operator for set return bad only in specific cases, it requires the type as declared in common. If this was now directly in schemas/expression, the multimap module would only partially initialize and crash, since it depends on set but set cannot be initialized either since it depends on expression which in turn depends on multimap.
This, to me, seems like the hierarchy of the modules is not really clear.
Resolution
In my view, evaluator modules such as logic.py, arithmetic.py, multimap.py or set.py are just extracted functionality of the evaluator itself. Thus, they are sub-modules of the evaluator. If the evaluator provides other classes as schemas, then I don't see a reason why required schemas for it's sub-modules aren't also simply provided.
In general, I have the view that while providing schemas is a good common practice (for example in the context of databases) they should be treated as "core" modules. Meaning, they should, if possible, be self-contained or only depend on other modules at the same or a lower level. To me, schemas represent a lower level than the evaluation since the latter clearly depends on the former, but not the other way around.
Likewise, arithmetic etc. operate either at the same level of the evaluator or between the evaluator and the schemas, because they're practically just specialized evaluators. But they, also, cannot exist without their respective schemata. In fact, they also use warning from schemas and use common from utils as well.
I think, all operators currently defined in evaluation modules such as logic.py, arithmetic.py, multimap.py or set.py should be moved to a respective schema file or to some operators.py that declares the specific operators as well as the full list.
If done properly, this would probably just change the interface from arithmetic.Operator.add to operators.Arithmetic.add.
But it would create a clear hierarchy with schemas being the ground truth or contract between modules. This of course would also mean that Bad would be moved out of utils.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by mapping imports among schemas/expression.py, utils/common.py, and the evaluator modules arithmetic.py, logic.py, multimap.py, and set.py. Review where Operator, Bad, and HashableDict are defined and used; done would require an agreed hierarchy that removes the described circular-import risk and establishes consistent schema ownership.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100