Moving all default settings to `.settings` file #3718

rkapka · 2018-01-21T23:34:52Z

Closes #3555

retailcoder · 2018-01-22T00:27:35Z

Just to be clear: this PR is moving all default settings values to the .settings file, correct?

rkapka · 2018-01-22T08:00:08Z

@retailcoder Yes 😀. I will edit the title.

# Conflicts: # RubberduckTests/RubberduckTests.csproj

retailcoder · 2018-01-24T22:10:55Z

AppVeyor is claiming a build error here, involving ParseTreeInspectionBase and DefTypeInspection

rkapka · 2018-01-24T22:26:16Z

Yeah, I didn't take a closer look that an inspection was added. The default severity is now inside the .settings file, so there is no constructor in ParseTreeInspectionBase taking a severity.

rkapka · 2018-01-24T22:44:06Z

I just noticed that I also moved the InspectionType into the .settings file, so at the moment it's specified there and in a property. Maybe the property could be defaulted to some value in the base class and be read from only if the inspection isn't present in the .settings file...

retailcoder · 2018-01-25T01:54:34Z

@rkapka but an inspection's InspectionType isn't a setting.. hmm, do we want them there though? Having inspection types in the settings makes it easier to visualize the groupings and distribution... I'm all for it! 👍

retailcoder · 2018-01-25T02:03:36Z

RetailCoder.VBE/Settings/WindowSettings.cs

-                return TodoExplorerVisibleOnStartup;
-            }
-            //Oh. Hello. I have no clue who you are...
-            return false;
        }


retailcoder · 2018-01-25T02:09:20Z

RetailCoder.VBE/Settings/CodeInspectionConfigProvider.cs

+            _defaultSettings = new DefaultSettings<CodeInspectionSettings>().Default;
+
+            var nonDefaultInspections = foundInspections
+                .Where(inspection => !_defaultSettings.CodeInspections.Select(x => x.Name).Contains(inspection.Name));


I think that's iterating the inspection settings twice. .Any() could help here I think:

var nonDefaultInspections = foundInspections .Where(inspection => !_defaultSettings.CodeInspections.Any(x => x.Name == inspection.Name));

Any also iterates, so I will extract the names to a HashSet to avoid this.

It does, but Select is first projecting all elements into an IEnumerable<string>, which Contains then iterates. Any skips the projection part; HashSet seems overkill.

This part got removed.

retailcoder · 2018-01-25T02:12:26Z

RetailCoder.VBE/Settings/CodeInspectionSettings.cs

        }

        public override int GetHashCode()
        {
            unchecked
            {
                var hashCode = Name?.GetHashCode() ?? 0;
-                hashCode = (hashCode * 397) ^ (int)Severity;


Holy crap, nice catch!! 😲

retailcoder

The amount of cleanup is impressive, some serious issues are fixed too, very good PR! I have a concern about the InspectionType though - see comments. I think this PR is the pefect opportunity to clear this up. Thanks!

Vogel612 · 2018-01-25T11:45:51Z

While InspectionTypes should not be a user-configurable setting, I think it's not a generally bad decision to make such settings configurable through our app settings.

This should also remove the tests on all the inspections that check for a certain InspectionType...
OTOH, this means that any plugged in inspections need to be configurable through this general app-configuration, which is not quite there yet.
Then again we're not really ready for any plugins quite yet...

AFAIK we do not overwrite the Inspection's InspectionType with values read from settings, so in itself it's not used and should be removed...

retailcoder · 2018-01-25T21:44:22Z

@Vogel612 the InspectionType isn't read from settings, but the inspection settings UI does use that property for grouping the inspections, so it can't just be removed from the model.

Ideally the config file would drive what the InspectionType is for each inspection, but then individual inspections would have some way of specifying an InspectionType if we don't have a default for it... but then, this makes a rather confusing API: when implementing a new inspection, you immediately pick up that you need to provide an InspectionType, but not that you need to have it in the .settings file.... catch-22?

rkapka · 2018-01-25T23:49:08Z

@retailcoder How about making InspectionType a non-abstract, get/set property and defaulting it to QualityIssues or whichever value is most reasonable? CodeInspectionConfigProvider will set the property in its constructor using default settings. That way it would behave similarly to Severity.

retailcoder · 2018-01-25T23:55:03Z

@rkapka that can work... We just don't want the inspection types to end up in the serialized xml config file.

Vogel612 · 2018-01-26T17:32:02Z

This will need to incorporate the new settings for #3717

…settings.

# Conflicts: # RubberduckTests/Inspections/ObjectVariableNotSetInspectionTests.cs

…o `.settings` file

rkapka · 2018-02-04T13:50:02Z

That would be all except leaving the inspection type setting out of the user-specific settings file. Excluding the property from serialization will prevent the .settings file from including the value in the default settings. I don't know how to solve this at the moment... It is possible to write a custom type converter and use SettingsSerializeAs.String to store a string representation of the setting in the .settings file, including the inspection type. But I am not sure how to embed such a string inside the XML setting for CodeInspectionSettings and if it is possible to serialize individual inspections as strings in the .settings file and as XML in the user-specific file.

For now the inspection type is visible in the user file, but it is ignored by the configuration provider. Any comments/ideas?

Vogel612 · 2018-02-06T22:57:58Z

Also fixes #2240

retailcoder · 2018-02-08T02:34:09Z

For now the inspection type is visible in the user file, but it is ignored by the configuration provider. Any comments/ideas?

I think that's more than good enough... the configuration file is intended to be accessed from the settings dialog.. I guess we can consider it by-design.

retailcoder

thanks for this 👍

rkapka added 7 commits January 13, 2018 23:01

Moved window settings to .settings file

a1a7483

Moved unit test settings to .settings file

7bee7f9

Moved general settings to .settings file

8305394

Moved inspection settings to .settings file

935b701

Merge remote-tracking branch 'upstream/next' into rkapka-master

84bb92c

Weird settings change after merge

9edf8ed

Default and user inspection settings will be merged together

e11b25b

rkapka added the PR-Status: Review Requested No more commits, PR is ready for the eyes that need to see it. label Jan 21, 2018

rkapka changed the title ~~Moving all settings to .settings file~~ Moving all default settings to .settings file Jan 22, 2018

rkapka mentioned this pull request Jan 23, 2018

Step one inspection #3717

Merged

Merge remote-tracking branch 'upstream/next' into rkapka-master

d927937

# Conflicts: # RubberduckTests/RubberduckTests.csproj

Moved severity of DefTypeStatementInspection to .settings file

3963c7a

retailcoder reviewed Jan 25, 2018

View reviewed changes

retailcoder requested changes Jan 25, 2018

View reviewed changes

rkapka added 2 commits January 31, 2018 00:31

Removed InspectionType default value from inspection classes

54f0c3f

Inspections will have their InspectionType assigned from application …

d9ae966

…settings.

rkapka removed the PR-Status: Review Requested No more commits, PR is ready for the eyes that need to see it. label Jan 31, 2018

rkapka added the PR-Status: WIP Pull request is work-in-progress, more commits should be expected. label Jan 31, 2018

rkapka added 5 commits February 2, 2018 00:10

Introduced InspectionProvider

8f066e1

Removed tests that verified inspection type.

b4926c7

Settings for experimental inspections will not be displayed

ac2e8be

Merge remote-tracking branch 'upstream/next' into rkapka-master

b1d96ff

# Conflicts: # RubberduckTests/Inspections/ObjectVariableNotSetInspectionTests.cs

Moved StepIsNotSpecifiedInspection and StepOneIsRedundantInspection t…

4594df1

…o `.settings` file

rkapka added the discussion label Feb 4, 2018

Merge branch 'next' into rkapka-master

c865400

Vogel612 mentioned this pull request Feb 6, 2018

Coupling issues in CodeInspectionSettings #2240

Closed

rkapka added the PR-Status: Review Requested No more commits, PR is ready for the eyes that need to see it. label Feb 7, 2018

retailcoder removed the PR-Status: WIP Pull request is work-in-progress, more commits should be expected. label Feb 8, 2018

retailcoder merged commit 8c57bed into rubberduck-vba:next Feb 8, 2018

retailcoder reviewed Feb 8, 2018

View reviewed changes

rkapka removed PR-Status: Review Requested No more commits, PR is ready for the eyes that need to see it. discussion labels Feb 8, 2018

rkapka mentioned this pull request Oct 6, 2018

Module without folder annotation #4399

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving all default settings to `.settings` file #3718

Moving all default settings to `.settings` file #3718

rkapka commented Jan 21, 2018

retailcoder commented Jan 22, 2018

rkapka commented Jan 22, 2018

retailcoder commented Jan 24, 2018 •

edited

Loading

rkapka commented Jan 24, 2018

rkapka commented Jan 24, 2018

retailcoder commented Jan 25, 2018

retailcoder Jan 25, 2018

retailcoder Jan 25, 2018 •

edited

Loading

rkapka Jan 25, 2018 •

edited

Loading

retailcoder Jan 25, 2018

rkapka Feb 4, 2018

retailcoder Jan 25, 2018

retailcoder left a comment

Vogel612 commented Jan 25, 2018

retailcoder commented Jan 25, 2018

rkapka commented Jan 25, 2018

retailcoder commented Jan 25, 2018

Vogel612 commented Jan 26, 2018

rkapka commented Feb 4, 2018

Vogel612 commented Feb 6, 2018

retailcoder commented Feb 8, 2018

retailcoder left a comment

Moving all default settings to .settings file #3718

Moving all default settings to .settings file #3718

Conversation

rkapka commented Jan 21, 2018

retailcoder commented Jan 22, 2018

rkapka commented Jan 22, 2018

retailcoder commented Jan 24, 2018 • edited Loading

rkapka commented Jan 24, 2018

rkapka commented Jan 24, 2018

retailcoder commented Jan 25, 2018

retailcoder Jan 25, 2018

Choose a reason for hiding this comment

retailcoder Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

rkapka Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

retailcoder Jan 25, 2018

Choose a reason for hiding this comment

rkapka Feb 4, 2018

Choose a reason for hiding this comment

retailcoder Jan 25, 2018

Choose a reason for hiding this comment

retailcoder left a comment

Choose a reason for hiding this comment

Vogel612 commented Jan 25, 2018

retailcoder commented Jan 25, 2018

rkapka commented Jan 25, 2018

retailcoder commented Jan 25, 2018

Vogel612 commented Jan 26, 2018

rkapka commented Feb 4, 2018

Vogel612 commented Feb 6, 2018

retailcoder commented Feb 8, 2018

retailcoder left a comment

Choose a reason for hiding this comment

Moving all default settings to `.settings` file #3718

Moving all default settings to `.settings` file #3718

retailcoder commented Jan 24, 2018 •

edited

Loading

retailcoder Jan 25, 2018 •

edited

Loading

rkapka Jan 25, 2018 •

edited

Loading