Autumn cleaning part 3 - split transactron/core.py #471
Conversation
|
There were needed some changes in transactron code to break cyclic references. Some of them are imports inside functions instead of top level imports. Affected functions:
The alternative is to use absolute names, which will be ugly in my opinion. |
transactron/core/transaction_base.py
Outdated
| from .method import Method | ||
| from .transaction import Transaction |
There was a problem hiding this comment.
This will be resolved by putting Method, Transaction and TransactionBase in a single file.
There was a problem hiding this comment.
I don't like the idea to put Method, Transaction and TransactionBase to single file. Such file will be big (over 500 lines).
There was a problem hiding this comment.
I start to think if our type hints are correct. In TransactionBase we make some assumptions about subclasses. We allow only Transaction and Method subclass, so we are leaking the abstraction. The type hint TransactionOrMethod is from the time before creating TransactionBase.
c87bb31
So in my opinion we should use TransactionBase type instead of TransactionOrMethod.
There was a problem hiding this comment.
So in my opinion we should use TransactionBase type instead of TransactionOrMethod.
No, this is wrong. TransactionBase is an open type, TransactionOrMethod is a closed one. Or: if you know that a TransactionOrMethod is not a Transaction, it must be a Method. There is no such guarantee for TransactionBase.
There was a problem hiding this comment.
Yes. There is no such guarantee on type level, but if we need it, then the code is wrongly designed. In base class these should be defined only code, which is common for all its subclasses. So limiting it to only TransactionOrMethod suggest, that it wouldn't be common for other subclasses, so it shouldn't be in base class.
There was a problem hiding this comment.
You seem to misunderstand. The transaction manager works on transactions and methods only, and the TransactionOrMethod type tells the typechecker exactly that. Your change messed this up, see e.g. lines 52-55 in manager.py.
tilk
added
refactor
|
Second proposition has been just uploaded.
Changes:
|
|
I think that this looks much better than the initial proposal. Would like to get a second opinion before deciding. |
| if isinstance(elem, Transaction): | ||
| return [elem] | ||
| else: | ||
| assert isinstance(elem, Method) |
There was a problem hiding this comment.
This is silly. Original type already told the typechecker that this is the only option here.
|
The big problem with this PR is that I see NO WAY to check what actual changes were made to the code, so there is no way to review them. |
| run = Signal() | ||
| m.d.comb += run.eq(run_val) | ||
| else: | ||
| assert isinstance(source, Transaction) |
There was a problem hiding this comment.
Again, you silently added this assert.
Yes, it is the problem and the reason why I wanted to have all split to small PR. I have two ideas:
|
Unfortunately, some of your later commits contain both moves and code changes, e.g. "Remove relation database". As you smuggled weakening of the type guarantees in the most fundamental code in this project there, I'm now becoming paranoid with regard to your changes. |
|
Ok. So I will try to split the changes to separate PR without any moves. |
Here is proposition to split core.py into smaller parts. Tests don't work yet (particularly I haven't started to solve circular imports yet). Please comment about split and naming.
Method, Transaction, TransactionBase, TransactionManager are very big classes, so I decided that each of them will be in separate file.