Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Separate config parsing from rest of the code #383
Separate config parsing from rest of the code #383
Changes from 1 commit
2e06d9e
ac78368
128fffb
696dc5f
98df345
01affef
5aa432d
652ad12
da7a6f2
e0bfd19
67d9ea9
f8d2ff9
1a8aeb2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theSydent
constructor alongside the existing config dictionary. Then you can gradually move bits of the configuration parsing into the newSydentConfig
, so your second commit could create simple likeDatabaseConfig
, and updateSqliteDatabase
to useDatabaseConfig
instead of the dictionary. And so on.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for taking time to look at this.
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!
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit odd that all the modules in
sydent/config
areBaseConfig
classes, except this one. I'd expectsydent/config/server.py
to hold some sort of server-specific config.Ideas (none of which I love):
BaseConfig
classes down into a submodule (sydent/config/sections
maybe)?SydentConfig
intosydent/__init__.py
SydentConfig
into some other module in the top-levelSydent
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'd be nice to get rid of this, and instead just have a
_parse_config
method which calledsection.parse_config(cfg)
on each ofgeneral
,email
, etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is pretty nasty. Can we just have
section.parse_config
return a boolean to indicate whether an update is needed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably do this after we parse all the sections, rather than after each section?