Reduce settings manager complexity by loading sections via DI #11398
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ChristophWurst
requested review from
rullzer,
nickvergessen,
MorrisJobke and
blizzz
blizzz
approved these changes
Sep 26, 2018
ChristophWurst
force-pushed
the
refactor/settings-manager-complexity
branch
from
e5901e3 to
6fe3b37
Compare
|
CI failure is unrelated. |
ChristophWurst
added
4. to release
Isn't it awesome how the automation knows two reviews are required and once done moves this ticket on the project board? 🚀 |
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst
force-pushed
the
refactor/settings-manager-complexity
branch
from
6fe3b37 to
f71ffc7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
For #11381 I need to add some more section to the server's default settings sections and therefore I stumbled over this massively overcomplicated class that build the settings sections for both personal and admin settings.
IMO it's a lot easier and cleaner to load these external classes via the DI container. This way changes to them don't have to be reflected in the settings manager (and they shouldn't -> single responsibility principle).