rubberduck-vba / Rubberduck

Every programmer needs a rubberduck. COM add-in for the VBA & VB6 IDE (VBE).
https://rubberduckvba.com
GNU General Public License v3.0
1.91k stars 299 forks source link

Existance of default settings in Rubberduck.Properties is not unit tested #4493

Open comintern opened 5 years ago

comintern commented 5 years ago

The change that moved the default user settings for RD to the Settings.settings in Rubberduck.Core (and elsewhere) was committed without unit tests to ensure that there are entries for new settings. It has been brought to my attention (twice now) that these are "required" for new inspections (note that this "requirement" is a coding convention - RD functions perfectly fine when inspections are allowed fall back to the default values provided by the class implementation).

If this is a convention that we intend to enforce, we need a unit test to enforce it similar to the test for the existence of the inspection name. It's unclear if the InspectionTypeIsAssignedFromDefaultSettingInConstructor is supposed to do this, because if it is, it doesn't work.

If the intention is that this file (and similar files throughout the project) holds every default setting, this also needs to be tested. We should not be creating a situation where a PR can pass an AV build with "required" changes missing.

rkapka commented 5 years ago

I was thinking that a code analyzer could enforce this, but if this can be unit tested then all the better.

rkapka commented 5 years ago

😕

I wanted to see what happens when you try to change user settings for an inspection that doesn't have settings in the Properties project and it seems that everything works fine... You can successfully change the severity and reset it to the default value. Either I remember it wrong or some PR fixed the problem in the meantime.

comintern commented 5 years ago

Are we saving any changes to the app config? If we are, isn't that problematic if multiple RD instances are running at the same time? For example, if I have RD open in both Excel and Word and they're sharing the same configuration file then reads should be safe. Writes are not safe at all - see this answer on SO.

retailcoder commented 5 years ago

@comintern the app.config settings mean to contain the would-otherwise-be-hard-coded defaults, which should be read-only.