GeezerVison

This assert is already handled by line 189 so this doesn't need to be here

This is already handled by 217

This is handled by 244

Covered by 441

Covered by 469

Covered by 998

Covered by 1025

Covered by 1251

This validation code is very similar throughout this script. It seems you should have a Validate type function instead of the same code repeated

why return instead of else?

Given the choice I prefer if-return, because:

• The reader knows that they can stop reading when they see return
• Less indentation and less horizontal programming
• Closer to pattern match/guard style, which I find helps the "exhaust all possibilities" mentality
• The return works as a forcing function to not juggle state

I see many instances in the code base that I imagine initially started as innocent if-elses that could have been if-returns, but whose complexity has since increased due to that choice:

• PowerShell/src/System.Management.Automation/engine/Modules/ImportModuleCommand.cs

  Lines 1698 to 1783 in 01634e4

  	if (this.ParameterSetName.Equals(ParameterSet_ModuleInfo, StringComparison.OrdinalIgnoreCase))
  	{
  	// Process all of the specified PSModuleInfo objects. These would typically be coming in as a result
  	// of doing Get-Module -list
  	foreach (PSModuleInfo module in ModuleInfo)
  	{
  	RemoteDiscoveryHelper.DispatchModuleInfoProcessing(
  	module,
  	localAction: delegate ()
  	{
  	ImportModule_ViaLocalModuleInfo(importModuleOptions, module);
  	SetModuleBaseForEngineModules(module.Name, this.Context);
  	},
  	cimSessionAction: (cimSession, resourceUri, cimNamespace) => ImportModule_RemotelyViaCimSession(
  	importModuleOptions,
  	new string[] { module.Name },
  	cimSession,
  	resourceUri,
  	cimNamespace),
  	psSessionAction: psSession => ImportModule_RemotelyViaPsrpSession(
  	importModuleOptions,
  	new string[] { module.Path },
  	null,
  	psSession));
  	}
  	}
  	else if (this.ParameterSetName.Equals(ParameterSet_Assembly, StringComparison.OrdinalIgnoreCase))
  	{
  	// Now load all of the supplied assemblies...
  	if (Assembly != null)
  	{
  	foreach (Assembly suppliedAssembly in Assembly)
  	{
  	ImportModule_ViaAssembly(importModuleOptions, suppliedAssembly);
  	}
  	}
  	}
  	else if (this.ParameterSetName.Equals(ParameterSet_Name, StringComparison.OrdinalIgnoreCase))
  	{
  	foreach (string name in Name)
  	{
  	PSModuleInfo foundModule = ImportModule_LocallyViaName(importModuleOptions, name);
  	if (foundModule != null)
  	{
  	SetModuleBaseForEngineModules(foundModule.Name, this.Context);
  	#if LEGACYTELEMETRY
  	TelemetryAPI.ReportModuleLoad(foundModule);
  	#endif
  	}
  	}
  	}
  	else if (this.ParameterSetName.Equals(ParameterSet_ViaPsrpSession, StringComparison.OrdinalIgnoreCase))
  	{
  	ImportModule_RemotelyViaPsrpSession(importModuleOptions, this.Name, null, this.PSSession);
  	}
  	else if (this.ParameterSetName.Equals(ParameterSet_ViaCimSession, StringComparison.OrdinalIgnoreCase))
  	{
  	ImportModule_RemotelyViaCimSession(importModuleOptions, this.Name, this.CimSession, this.CimResourceUri, this.CimNamespace);
  	}
  	else if (this.ParameterSetName.Equals(ParameterSet_FQName, StringComparison.OrdinalIgnoreCase))
  	{
  	foreach (var modulespec in FullyQualifiedName)
  	{
  	RequiredVersion = modulespec.RequiredVersion;
  	MinimumVersion = modulespec.Version;
  	MaximumVersion = modulespec.MaximumVersion;
  	BaseGuid = modulespec.Guid;
  	PSModuleInfo foundModule = ImportModule_LocallyViaName(importModuleOptions, modulespec.Name);
  	if (foundModule != null)
  	{
  	SetModuleBaseForEngineModules(foundModule.Name, this.Context);
  	}
  	}
  	}
  	else if (this.ParameterSetName.Equals(ParameterSet_FQName_ViaPsrpSession, StringComparison.OrdinalIgnoreCase))
  	{
  	ImportModule_RemotelyViaPsrpSession(importModuleOptions, null, FullyQualifiedName, this.PSSession);
  	}
  	else
  	{
  	Dbg.Assert(false, "Unrecognized parameter set");
  	}

• PowerShell/src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

  Lines 6351 to 6506 in 01634e4

  	if (assemblyToLoad != null)
  	{
  	// Figure out what to use for a module path...
  	if (!string.IsNullOrEmpty(fileName))
  	{
  	modulePath = fileName;
  	}
  	else
  	{
  	modulePath = assemblyToLoad.Location;
  	}
  	// And what to use for a module name...
  	if (string.IsNullOrEmpty(moduleName))
  	{
  	moduleName = "dynamic_code_module_" + assemblyToLoad.GetName();
  	}
  	if (importingModule)
  	{
  	// Passing module as a parameter here so that the providers can have the module property populated.
  	// For engine providers, the module should point to top-level module name
  	// For FileSystem, the module is Microsoft.PowerShell.Core and not System.Management.Automation
  	if (parentModule != null && InitialSessionState.IsEngineModule(parentModule.Name))
  	{
  	iss.ImportCmdletsFromAssembly(assemblyToLoad, parentModule);
  	}
  	else
  	{
  	iss.ImportCmdletsFromAssembly(assemblyToLoad, null);
  	}
  	}
  	assemblyVersion = GetAssemblyVersionNumber(assemblyToLoad);
  	assembly = assemblyToLoad;
  	// If this is an in-memory only assembly, add it directly to the assembly cache if
  	// it isn't already there.
  	if (string.IsNullOrEmpty(assembly.Location))
  	{
  	if (! Context.AssemblyCache.ContainsKey(assembly.FullName))
  	{
  	Context.AssemblyCache.Add(assembly.FullName, assembly);
  	}
  	}
  	}
  	else
  	{
  	// Avoid trying to import a PowerShell assembly as Snapin as it results in PSArgumentException
  	if ((moduleName != null) && Utils.IsPowerShellAssembly(moduleName))
  	{
  	trySnapInName = false;
  	}
  	if (trySnapInName && PSSnapInInfo.IsPSSnapinIdValid(moduleName))
  	{
  	PSSnapInInfo snapin = null;
  	#if !CORECLR
  	// Avoid trying to load SnapIns with Import-Module
  	PSSnapInException warning;
  	try
  	{
  	if (importingModule)
  	{
  	snapin = iss.ImportPSSnapIn(moduleName, out warning);
  	}
  	}
  	catch (PSArgumentException)
  	{
  	//BUGBUG - brucepay - probably want to have a verbose message here...
  	;
  	}
  	#endif
  	if (snapin != null)
  	{
  	importSuccessful = true;
  	if (string.IsNullOrEmpty(fileName))
  	modulePath = snapin.AbsoluteModulePath;
  	else
  	modulePath = fileName;
  	assemblyVersion = snapin.Version;
  	// If we're not supposed to load the types files from the snapin
  	// clear the iss member
  	if (!loadTypes)
  	{
  	iss.Types.Reset();
  	}
  	// If we're not supposed to load the format files from the snapin,
  	// clear the iss member
  	if (!loadFormats)
  	{
  	iss.Formats.Reset();
  	}
  	foreach (var a in ClrFacade.GetAssemblies())
  	{
  	if (a.GetName().FullName.Equals(snapin.AssemblyName, StringComparison.Ordinal))
  	{
  	assembly = a;
  	break;
  	}
  	}
  	}
  	}
  	if (importSuccessful == false)
  	{
  	if (importingModule)
  	{
  	assembly = Context.AddAssembly(moduleName, fileName, out error);
  	if (assembly == null)
  	{
  	if (error != null)
  	throw error;
  	found = false;
  	return null;
  	}
  	assemblyVersion = GetAssemblyVersionNumber(assembly);
  	if (string.IsNullOrEmpty(fileName))
  	modulePath = assembly.Location;
  	else
  	modulePath = fileName;
  	// Passing module as a parameter here so that the providers can have the module property populated.
  	// For engine providers, the module should point to top-level module name
  	// For FileSystem, the module is Microsoft.PowerShell.Core and not System.Management.Automation
  	if (parentModule != null && InitialSessionState.IsEngineModule(parentModule.Name))
  	{
  	iss.ImportCmdletsFromAssembly(assembly, parentModule);
  	}
  	else
  	{
  	iss.ImportCmdletsFromAssembly(assembly, null);
  	}
  	}
  	else
  	{
  	string binaryPath = fileName;
  	modulePath = fileName;
  	if (binaryPath == null)
  	{
  	binaryPath = System.IO.Path.Combine(moduleBase, moduleName);
  	}
  	BinaryAnalysisResult analysisResult = GetCmdletsFromBinaryModuleImplementation(binaryPath, manifestProcessingFlags, out assemblyVersion);
  	detectedCmdlets = analysisResult.DetectedCmdlets;
  	detectedAliases = analysisResult.DetectedAliases;
  	}
  	}
  	}

• PowerShell/src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

  Lines 3646 to 3715 in 01634e4

  	Dbg.Assert(moduleName != null, "GetModuleSpecification should guarantee that moduleName != null");
  	foreach (PSModuleInfo module in context.Modules.GetModules(new string[] { "*" }, false))
  	{
  	if (moduleName.Equals(module.Name, StringComparison.OrdinalIgnoreCase))
  	{
  	if (!moduleGuid.HasValue || moduleGuid.Value.Equals(module.Guid))
  	{
  	if (requiredModule.RequiredVersion != null)
  	{
  	if (requiredModule.RequiredVersion.Equals(module.Version))
  	{
  	result = module;
  	loaded = true;
  	break;
  	}
  	wrongVersion = true;
  	}
  	else if (requiredModule.Version != null)
  	{
  	if (requiredModule.MaximumVersion != null)
  	{
  	if (GetMaximumVersion(requiredModule.MaximumVersion) >= module.Version && requiredModule.Version <= module.Version)
  	{
  	result = module;
  	loaded = true;
  	break;
  	}
  	else
  	{
  	wrongVersion = true;
  	}
  	}
  	else if (requiredModule.Version <= module.Version)
  	{
  	result = module;
  	loaded = true;
  	break;
  	}
  	else
  	{
  	wrongVersion = true;
  	}
  	}
  	else if (requiredModule.MaximumVersion != null)
  	{
  	if (GetMaximumVersion(requiredModule.MaximumVersion) >= module.Version)
  	{
  	result = module;
  	loaded = true;
  	break;
  	}
  	else
  	{
  	wrongVersion = true;
  	}
  	}
  	else
  	{
  	result = module;
  	loaded = true;
  	break;
  	}
  	}
  	else
  	{
  	wrongGuid = true;
  	}
  	}
  	}

• PowerShell/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs

  Lines 275 to 325 in c1c5344

  	if (x == null && y == null)
  	{
  	result = true;
  	}
  	else if (x != null && y != null)
  	{
  	if (x.Name != null && y.Name != null)
  	{
  	result = x.Name.Equals(y.Name, StringComparison.OrdinalIgnoreCase);
  	}
  	else
  	{
  	result = true;
  	}
  	if (result)
  	{
  	if (x.Guid.HasValue && y.Guid.HasValue)
  	{
  	result = x.Guid.Equals(y.Guid);
  	}
  	}
  	if (result)
  	{
  	if (x.Version != null && y.Version != null)
  	{
  	result = x.Version.Equals(y.Version);
  	}
  	else if (x.Version != null || y.Version != null)
  	{
  	result = false;
  	}
  	if (x.MaximumVersion != null && y.MaximumVersion != null)
  	{
  	result = x.MaximumVersion.Equals(y.MaximumVersion);
  	}
  	else if (x.MaximumVersion != null || y.MaximumVersion != null)
  	{
  	result = false;
  	}
  	if (result && x.RequiredVersion != null && y.RequiredVersion != null)
  	{
  	result = x.RequiredVersion.Equals(y.RequiredVersion);
  	}
  	else if (result && (x.RequiredVersion != null || y.RequiredVersion != null))
  	{
  	result = false;
  	}
  	}
  	}

To my mind, using else just before the end of a function is a useless use of else.

Does it make sense to default this to $moduleName, $guid to $actualGuid, and $version to $actualVersion since that is used by most (all?) of the tests?

Yes I think that makes sense 👍

Worth noting that the $moduleName one relies on dynamic scope. I kept the other parameters, since I think it's less confusing. But it's not quite consistent with $moduleName.

If all the negative test cases result in a terminating error, it may be simpler to have the FQErrorId as part of the testcase variation so this would simply be

$sb | Should -Throw -ErrorId $ErrodId

Just realised this second suggestion doesn't work just yet; Import-Module -MaximumVersion -MinimumVersion throws a different error to Import-Module @{ ModuleVersion = ; MaximumVersion = }.

That's what #7347 changes.

Ok