NetCore warning removal

[MiYanni]

Description

Fix for: Azure#5565

Checklist

• I have read the Submitting Changes section of CONTRIBUTING.md
• The title of the PR is clear and informative
• The appropriate change log has been updated
• The PR does not introduce breaking changes
• If applicable, the changes made in the PR have proper test coverage
• For public API changes to cmdlets:
  • the changes have gone through a cmdlet design review
  • the cmdlet markdown files were generated using the platyPS module

[markcowl]

Hmm, I would think this would be used throughout their client models, as it should be used in any reference to another network resource

[MiYanni]

This file itself is NetCore specific. And, nothing was using that interface in the NetCore module code... So, I don't know.

[markcowl]

These calls are really a lot less descriptive now, and this test is more difficult to maintain. We should revert changes related to the application of this rule.

[MiYanni]

See comment below.

[markcowl]

same. I've seen this applied innocuously in a few places, but generally, this seems like a bad rule.

[MiYanni]

It is exposing a code smell. If you want patterns like this, it is better to make a POCO type that just has these things as properties. Then, make that type the parameter to the method. For CheckCmdletParameterAttributes, you could do this:

public CmdletParameterAttributes
{
  public Type CmdletType {get; set;}
  public string ParameterName {get; set;}
  public bool IsMandatory {get; set;}
  public bool ValueFromPipelineByName {get; set;}
}

// In the caller:
var cmdletParameterAttributes = new CmdletParameterAttributes
{
  CmdletType = type,
  ParameterName = "ServerName",
  IsMandatory = true,
  ValueFromPipelineByName = false
};
UnitTestHelper.CheckCmdletParameterAttributes(cmdletParameterAttributes);

This is the general C# style for handling a bunch of parameters for methods. Then, you always have to be explicit when initializing their value.

As seen here, a [Flags] enum is also a good option.
https://softwareengineering.stackexchange.com/questions/307773/specify-optional-parameter-names-even-though-not-required

Again, I'm not trying to redesign all our code. I'm just stating why this suggestion is generally not a bad one. I can, however, enable it to use named arguments for literal values, which seems to be the style that they are following here.

[markcowl]

Hmmm, so the point here was to test application of the explicit type converter for these types. It's possible the compiler might optimize these out, but likely only when the explicit conversion is doing what it is supposed to.

[markcowl]

this would apply to the other applications of this

[MiYanni]

Boxing here would make no difference if you are doing (Thing)null == null which is essentially what this is doing, right? You can't actually cast null. It just gets boxed.
https://dotnetliberty.com/index.php/2015/10/11/cast-to-object-before-comparing-to-null/
AFAIK, null is of type Object. Every reference type derives of Object. So, even though you are casting to some reference type, it'll get compared as boxed Object when doing the null check. I could be wrong on that, though.

As an aside, obviously Assert.Null(null) will always be true, so optimizing this just looks goofy.

[markcowl]

seems like these are still needed

[markcowl]

so, this should be used in resource template deployment

[MiYanni]

I didn't comment this out. I just deleted it since the entire file was commented out.

[markcowl]

??

[MiYanni]

<ProjectReference Include="..\..\..\Common\Commands.Common.Storage\Common.Storage.Netcore.csproj" />
That project holds the common reference to Storage at the moment. It was a later goal to remove that reliance, but for now, it makes sure that anything using it uses the same version of the management library.

[markcowl]

See my email, I think we need to restructure this and exclude generated code before we can merge.