Features/refactoring common

[wokhan]

Hi,

As I'm trying to follow my own guidelines (i.e. not pushing to master directly), here is a huge PR including all refactorings I could came up with as of now (some are still missing).

I didn't clean all warnings, but some (regading null values handling) are still to be fixed (others can wait). Regarding this: the Common project now uses the "new" Nullable management of c# 8.0. Only variables that could be null (i.e. by design) must be declared as nullable (using "?"), others should never be null. That way it's easier to now when a "null" ref can be expected or not. I also saw that configuration loading was broken (didn't check when it broke). I quickly implemented a small fix, which is clearly not the best way to go (overriding setting properties' provider to match the custom one). I'll add an issue to migrate to a proper .Net Core configuration handling (with json files), but if it works for you this way, it should be ok to wait a bit. BTW I had to remove config from %APPDATA%/roaming and local as it was complaining about some properties not being saved in the right place (I guess because some moved from user to machine or vice versa). Please tell me if you encounter the same issue!

Sidenote : TestFindMatchingFilterInfo and TestFindMatchingFilterInfo2Boot3 cannot work due to localization (in AreEquals statement), @harrwiss did you have a look at those previously? I don't remember ;-)

Thanks!

[wokhan]

BTW, I'll update CONTRIBUTING.MD file to add some rules (which should end up in .editorconfig), as use var when type is already explicit, case handling, and so on... to follow with things introduced / fixed in this PR.

[harrwiss]

Hi @wokhan, seems merge conflicts with master must first be resolved by you in the pull request branch (I'm also not authorized to do that) before it can be further reviewed 😏

Sidenote : TestFindMatchingFilterInfo and TestFindMatchingFilterInfo2Boot3 ... did you look into that?

Yes I'm aware of that, the problem is that filterKey's are not stable - therefore I added the fixme comment below some time ago and set the nunit test category to [FixmeCategory] and methods to [Test, ManualTestCategory] since they are still usefull for manual test runs.

/// Note that filterId and even filterKey is generated at runtime and therefore may change 
/// after a reboot etc.
/// 
/// FIXME: Since filter names and even keys and ids can change they are not reliable. Either create test rules on the fly or get some random rules
/// from the netsh output.
/// 
[FixmeCategory]
public class NetshHelperTest : NUnitTestBase

[wokhan]

Hi @harrwiss, indeed, sorry I missed this (and forgot that... regarding fixmecategory ;) ). Checking the conflicts right now. Please note that I know I should have split my modifications into specific commits instead of 3 huge ones, but I sometimes cannot stop updating code when I see something.... I'll have to improve that ;-)

[wokhan]

I added a micro-fix and resolved the conflicts (obviously keeping your code for Status.xaml/cs and InstallHelper, along with the Enum you introduced for process names - which should not be in ProcessHelper in fact, this one requiring to stay generic). As I'm still getting used to merging on Visual Studio, I hope I didn't miss any other modification... Thanks!

[harrwiss]

Ok, I'm going to review what I can this afternoon 😅

Yeah, it's a good practice to re-merge from master regularely - pays off when working with many contributors especially 😄

[wokhan]

Note to self: follow @harrwiss advice regarding regular merge from master Thank you! I tried not to change too much things at once (...), I hope you won't feel lost against the "new" classes hierarchy (and some other things I might have been changing).

[harrwiss]

Had to investigate and fix first various compile errors mainly in InstallHelper, ProcessHelper, Status.xaml which probably where introduced with the merge and because some methods where moved in the previous refactoring 👎

Also added a ProcessNames class which contains the known process names as another atempt to use checked parameters instead of plain strings - to be reviewed by you 😏

Did not have time to review the rest yet - will start now....

[wokhan]

You shouldn't have any issue with said files as I had them and fixed them all :-/

[wokhan]

Ok now I get it... my updates for the conflicts have not been pushed.... I'm deeply sorry about this @harrwiss. I think I forgot to index corresponding modifications and didn't check afterwards.

[harrwiss]

Ohh, dont worry, that sounds familiar ;-)

[wokhan]

Ha ha there was a little "isDebugEnabled" (instead of IsDebugEnabled) laying around in LogHelper. I modified directly in master... (see latest commit).