Separate config parsing from rest of the code

[Azrenbeth]

Seperates the config parsing from the rest of the code

Parse the config before doing anything else, similar to Synapse

• Better seperates out the different config sections
• Removes calls to confusing ConfigParser.get() from the rest of the code
• Allows for easy migration to different config format in the future (e.g. YAML like Synapse's)

All of the parsing should be exactly the same as it was before, but just done earlier

Sydent.get_branded_template() no longer has a deprecated_template_name argument

• All calls to this method now check the deprecated option first
• Needed to change all calls to this anyway but I'm unsure if this was correct approach

Can be reviewed commit to commit

[richvdh]

@Azrenbeth: generally it's best to use the imperative for commit comments: "Update .gitignore" rather than "updates ...". See https://chris.beams.io/posts/git-commit/#imperative for more info.

[richvdh]

I very much applaud your efforts here - the general direction seems to make a lot of sense.

However, as I explain below (#383 (comment)), it's really tricky to review as it stands. I've made a bunch of comments on things that stand out, but because so much is changing at once, it's going to take me forever to reconcile it all. Please could you have another go at it - maybe even split it up into more PRs, but certainly rearrange the commits so that we can review each one independently.

[richvdh]

ok, so: you've said "Can be reviewed commit to commit" - but that only works when each commit actually stands alone, or almost alone, in its own right. In this case (ac78368) you've added a new lump of code which isn't used anywhere, so that doesn't seem to make sense. As a reviewer when I look at this commit on its own, should I assume this is all new code, and review it accordingly? And then: the individual config classes (CryptoConfig and friends) don't exist yet, so I can't really review anything touching that.

So the long and short of it is that I don't think this can be reviewed commit to commit.

Splitting up big changes so that they can be reviewed easily can be challenging, and generally takes a bit of thinking about. What you're aiming for is a change that is simple enough that anyone can look at it for a couple of minutes - even if they haven't been following the progress of change in the project or even in the branch - and say "sure, this is obviously correct". That approach helps anyone else looking at the changes (including yourself when you come back to it in a few weeks' time), but is also a useful way for you as the author to be sure that the changes you are making are correct (and will also help track down where any problems cropped up).

The implication is that I could take any of the commits on your branch, and the code would all work properly: I could fire up a working application, or run all the tests and lint and expect them to pass. You don't necessarily need to actually run them all, but it's a useful guideline to consider.

So it's generally not as simple as "add a load of new code in one commit, then switch to it in later commits". Rather, you need to look for individual bits of functionality you can move around.

As an example, in this case, you might start in your first commit by creating an empty SydentConfig class and passing it into the Sydent constructor alongside the existing config dictionary. Then you can gradually move bits of the configuration parsing into the new SydentConfig, so your second commit could create simple like DatabaseConfig, and update SqliteDatabase to use DatabaseConfig instead of the dictionary. And so on.

[Azrenbeth]

Thank you very much for taking time to look at this.

So it's generally not as simple as "add a load of new code in one commit, then switch to it in later commits". Rather, you need to look for individual bits of functionality you can move around.

I see your point about making changes that work standalone and I will have another go at this where I move individual bits of functionality in peices. I can see how that would be much easier to review!

The implication is that I could take any of the commits on your branch, and the code would all work properly: I could fire up a working application, or run all the tests and lint and expect them to pass. You don't necessarily need to actually run them all, but it's a useful guideline to consider.

Thank you, that's really clear advice!

I'm sorry for taking unneccessary amounts of your time up, and I will try to make my next attempt very clear!

[richvdh]

@Azrenbeth please don't apologise. This is great work, and it's not at all obvious how best to structure these things. You're doing great!

[Azrenbeth]

Replaced by #385