Use Unicode functions consistently and remove helper extension methods that were made public

[jnm2]

Fixes #72

This one is not urgent. I have improvements I'd like to do in other PRs.

Please review carefully in case I made a mistake. The test suite does execute the affected code paths, and I checked that HidDevice.Description remained the same.

I also removed the public Extensions class with ToUTF8String and ToUTF16String because I don't think it was meant to be public. It has the unfortunate side effect of appearing as an extension method on byte arrays in intellisense for all projects that use HidLibrary. It was added in aad5c64. If any consuming projects did directly call these extension methods, they will fail to recompile against the new version of HidLibrary. If a consuming assembly is not recompiled but runs using the new version of HidLibrary anyway, the .NET runtime will throw MissingMethodException if the consuming project did directly call these extension methods.
An alternative is to obsolete them and put [EditorBrowsable(EditorBrowsableState.Never)] on them so that they stop polluting intellisense for all byte arrays in consuming projects.

[amullins83]

I'd really love to hear @mikeobrien's thoughts on this before merging, just to make sure we're clear on which path we want to take moving forward. It would be interesting to find out whether anyone has tried to use those extensions outside the library, but I doubt we will know until someone gets mad 😂 .

[amullins83]

I have no problem with these changes. Seems quite a bit cleaner.

[jnm2]

It's a very easy change to obsolete and hide those extension methods. I made a more radical change initially that we'd only keep if you're both super comfortable with the disclaimers, but I'm really expecting to push a revision that brings them back as obsolete.

[jnm2]

I modified the original commits to no longer delete things in Extensions.cs, and then I added three new commits.

Unfortunately, both Visual Studio's and ReSharper's intellisense still show the extension methods even though they have EditorBrowsable(Never). I filed a VS bug for this (dotnet/roslyn#47551). I tested this using a new project in a new solution that directly referenced the HidLibrary.dll file.

Would you like to make them non-extension methods? This would get them to stop showing up in byte array intellisense. It would not be a binary-breaking change: no MissingMethodException for assemblies consuming HidLibrary that run against the new version without being recompiled. It would be a source-breaking change: projects consuming HidLibrary would stop compiling once they start pulling in the new version of HidLibrary. Instead of the obsoletion message explaining that the extension method is deprecated, they'd get "CS1061 'byte[]' does not contain a definition for 'ToUTF16String' and no accessible extension method 'ToUTF16String' accepting a first argument of type 'byte[]' could be found (are you missing a using directive or an assembly reference?)"

[jnm2]

Good news! The Roslyn team fixed this very fast and Visual Studio 16.8 will respect EditorBrowsable(Never) on the extension methods the way I hoped! I struck out the text above that no longer applies.

That answers the last outstanding question, I think?

[mikeobrien]

@amullins83 @jnm2 hey, sorry for the late reply. IMO depending on extension methods in a 3rd party library always comes with the risk that they can be changed, esp if they aren't documented as part of the official API. So I don't see any problem with pulling them. Plus this project doesn't even have official API doc's so we have even more latitude.... ;) I guess we should bump the major version since we are following semver and this may be a breaking change for some.

[mikeobrien]

Major/minor version is set here: https://github.com/mikeobrien/HidLibrary/blob/master/azure-pipelines.yml#L5

[jnm2]

@mikeobrien I ended up leaving them in; would pulling them be your preference? I'm thinking the major version update could be saved for something more exciting then.

[mikeobrien]

Ah ok cool, however you have it is fine then. Honestly I'm pretty disconnected from this project so don't have much of a preference on anything. ;) Thx for the PR!

[jnm2]

Cool! @amullins83 @mikeobrien Think it's ready to merge?