Skip to content
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

Invert project order for RD.CodeAnalysis and make RD.SettingsProvider more useful #4914

Merged
merged 5 commits into from Apr 20, 2019

Conversation

Vogel612
Copy link
Member

This cleans up Settings and their loading behaviour a bit and moves most of the interfaces relating to it into Rubberduck.SettingsProvider.
The Defaults are now loaded differently, registration is probably utterly broken, though.

Furthermore this extracts the CodeInspectionSettings defaults to RD.CodeAnalysis to make them accessible for the loader there.
It also inverts the project interdependence between RD.Core and RD.CodeAnalysis in preparation for a new large feature involving dependency graphs.

As it stands the Defaults need a bit of cleanup and revisiting of the loading mechanism they use. This should unify how settings work and make specialized SettingsProviders superfluous (or at least significantly easier to implement)

For that purpose we cleaned up Settings and their loading behaviour a bit and moved most of the interfaces relating to it
into Rubberduck.SettingsProvider. The Defaults are now loaded differently, registration is probably utterly broken, though.

Furthermore this extracts the CodeInspectionSettings defaults to RD.CodeAnalysis to make them accessible for the loader there.
This moves the Consumer-Visible interfaces for Settings into the SettingsProvider project.
Additionally a base-implementation for a ConfigurationService is provided.
It uses a clear reflection-based mechanism to load the defaults for a given settings type
from a settings class. The type is provided as a Type-Argument to the constructor.

This structure should allow easily extensible and customizable settings loading with caching and defaults.
@codecov
Copy link

codecov bot commented Apr 17, 2019

Codecov Report

Merging #4914 into next will increase coverage by 0.01%.
The diff coverage is 44.22%.

@@            Coverage Diff             @@
##             next    #4914      +/-   ##
==========================================
+ Coverage   64.55%   64.56%   +0.01%     
==========================================
  Files        1063     1065       +2     
  Lines       35564    35573       +9     
==========================================
+ Hits        22955    22965      +10     
+ Misses      12609    12608       -1
Impacted Files Coverage Δ
...rduck.Core/UI/Settings/GeneralSettingsViewModel.cs 62.66% <ø> (ø) ⬆️
...nspections/Concrete/UseMeaningfulNameInspection.cs 100% <ø> (ø) ⬆️
...nspections/Concrete/HungarianNotationInspection.cs 100% <ø> (ø) ⬆️
...erduck.Core/UI/Settings/WindowSettingsViewModel.cs 0% <ø> (ø) ⬆️
Rubberduck.JunkDrawer/Output/StringExtensions.cs 87.5% <ø> (ø)
Rubberduck.CodeAnalysis/CodeMetrics/CodeMetric.cs 33.33% <ø> (ø)
Rubberduck.Core/Settings/UserSettings.cs 100% <ø> (ø) ⬆️
...alysis/Inspections/Helper/VariableNameValidator.cs 100% <ø> (ø)
....Core/UI/Settings/AutoCompleteSettingsViewModel.cs 86.27% <ø> (ø) ⬆️
...eAnalysis/Settings/WhitelistedIdentifierSetting.cs 80% <ø> (ø)
... and 66 more

@Vogel612 Vogel612 marked this pull request as ready for review April 18, 2019 19:41
@Vogel612
Copy link
Member Author

This PR adresses the I/O stall observed in #4905

Copy link
Contributor

@MDoerner MDoerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants