Remove setattr with kwargs from HomeServer class

[ShadowJonathan]

Change @cache_in_self to use _-prefixed attributes.

Cleanup some misc direct-attribute usages (to get_X() ones)

Add version_string attribute to HomeServer

Fix some tests that acted wonky because of all of this.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

Signed-off-by: Jonathan de Jong <jonathan@automatia.nl>

[ShadowJonathan]

If you're going to inject cache-specific attributes, at least be explicit about it

also, deduplicated code for 2 cases down below where it needs to be injected just after creating the homeserver objects and before any branch logic

[ShadowJonathan]

datastore=datastore was added previously, but get_datastore() is not annotated with @cache_in_self, so i'm including this here for any fucky sideeffects it might have

[ShadowJonathan]

Unless there is another class that is also exactly called TestHomeServer within the test framework, this is more idiomatic

[ShadowJonathan]

snake_case is more idiomatic for python

[clokep]

Change @cache_in_self to use _-prefixed attributes.

Cleanup some misc direct-attribute usages (to get_X() ones)

I'm not sure that accessing these directly was "wrong" -- I think it was designed to allow for this direct usage. Note that #8060 might have some info on the current implementation.

Add version_string attribute to HomeServer

To clarify -- this PR makes it so that version_string is an explicit parameter to HomeServer instead of being dynamically added via kwargs / setattr.

[ShadowJonathan]

Could change that to this for better clarification:

Suggested change

	# get_attr -> _attr
	# get_variable() -> _variable

[ShadowJonathan]

To clarify -- this PR makes it so that version_string is an explicit parameter to HomeServer instead of being dynamically added via kwargs / setattr.

Yes, and make it so that any attributes (that're later referred to) are not dynamically set inside __init__, this is to prepare for future mypy and typing soundness and consistency

I'm not sure that accessing these directly was "wrong" -- I think it was designed to allow for this direct usage.

Yes, but seeing that in code somewhere else makes it not "vaguely apparent" that it is not a normal variable, if it's prefixed with _, it gets seen as "oh, its an internal variable", opposed to having no underscore prefix, with which it'll be seen as an atomic variable, and not a weirdly-cached one. ("weird" in effect compared to conventional python)

Also, I'm trying to help a bit with enforcing an style, get_x() over .x

[anoadragon453]

I'm liking these changes, but have questions about others. However, my main concern is that there's a lot going on here, and it's all happening in one commit.

I think this PR would be a lot easier to review if each class of change were broken up into its own commit, or its own PR, so that discussion on each change could be compartmentalised.

[ShadowJonathan]

@anoadragon453 I could split it into;
A. the changes that affect the HomeServer __init__ use (removing kwargs and making cache injection explicit)

And then follow-up with

B. the changes that prefix the @cache_in_self attribute to use _-prefixes, and move all uses (that dont set the attribute) to use corresponding get_attr() cached functions.

I'd close this pull request and cherrypick the changes to a branch and PR change A, after that passes, i'd PR B, would that work?

[anoadragon453]

@ShadowJonathan Sounds like a solid plan, thanks!