Refactor out state delta handling into its own class #4917
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4917 +/- ##
===========================================
+ Coverage 78.01% 78.02% +<.01%
===========================================
Files 326 328 +2
Lines 34058 34067 +9
Branches 5621 5620 -1
===========================================
+ Hits 26572 26581 +9
- Misses 5867 5868 +1
+ Partials 1619 1618 -1 |
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class UserDirectoryHandler(object): | ||
| class UserDirectoryHandler(StateDeltasHandler): |
There was a problem hiding this comment.
composition > inheritance for this sort of thing, imho
There was a problem hiding this comment.
sure, that could be a good refactor later. Right now, I'm just splitting up what Matthew wrote.
There was a problem hiding this comment.
I don't think it will happen later. I'm not too bothered about this particular nit, but as a general principle, I don't think that we should be merging stuff into develop which wouldn't normally pass review just because Matthew wrote it in a hurry.
I'm sorry if you've been given a pile of code to try and sort out, but I am strongly averse to adding to our technical debt by adding essentially unreviewed code to develop.
There was a problem hiding this comment.
Sure, that's why this code is being reviewed. And, yes, I am a staunch believer in the composition > inheritance thing, which -- if I wrote it -- I might well have investigated how to do it. But, I didn't write it, but the code works, and it matches the rest of the inheritance-based storage/handler APIs, so there's no reason going back and thinking about how to make this API composition-friendly if we're not doing it across everything else that's inheritance based.
There was a problem hiding this comment.
my impression was that changing to composition here would be approximately a three line change, but if it's more complicated than that, I'm happy to concede.
* develop: (141 commits) Make federation endpoints more tolerant of trailing slashes v2 (#4935) Fix ClientReplicationStreamProtocol.__str__ (#4929) Fix bug where read-receipts lost their timestamps (#4927) Use an explicit dbname for postgres connections in the tests. (#4928) Fix nginx example in ACME doc. (#4923) Refactor out state delta handling into its own class (#4917) Newsfile Use yaml safe_load Allow newsfragments to end with exclamation marks! (#4912) Some more porting to HomeserverTestCase and remove old RESTHelper (#4913) Clean up backoff_on_404 and metehod calls Update changelog.d/4908.bugfix Update Apache Setup To Remove Location Syntax (#4870) isort Newsfile Fix typo and add description Deny peeking into rooms that have been blocked Rejig testcase to make it more extensible Remove debug Add tests ...
The new stats module uses some of the common components, so it's broken up into its own module.