If the only thing it's checking is the edition, maybe it should be called SkipEditionCheck ? Hmm - but maybe it should override the version check too? Then compatibility makes sense.

I agree! My concern is that SkipCompatibilityCheck is what is given in the RFC.

I think it's fine to change it to SkipEditionCheck as it's more specific

I'll update the RFC to reflect this change

Why change this from var?

I can change it back if desired, but my reasoning is:

• The return type of GetAvailableViaCimSessionCore(...) isn't inferable with only that line, or even only that file
• Reading the code without an IDE is something we have to do relatively often and is impaired by non-obvious var
• Being explicit about what's expected as a return type means:
  • Compile-time errors from any changes occur on that line (I've seen type inference push them into functions and different files)
  • There's a type-as-contract concept at work, which explicitly defines what's expected and promotes superclasses as a sort of "contract loosening"

But that is definitely all opinion! As I say, I can revert it if the previous style is preferred.

Same comment as Get-Module

Why not just do & "Test-$ModuleName" | Should -Be $Result

Ah much nicer!

Why not just do {Import-Module $ModuleName -ErrorAction Stop; & "Test-$ModuleName"} | Should -Throw -ErrorId "Modules_PSEditionNotSupported,Microsoft.PowerShell.Commands.ImportModuleCommand"

Again & "Test-$ModuleName" | Should -Be $Result

I think it's fine to change it to SkipEditionCheck as it's more specific

It seems that the two are not mutually exclusive. -PSEdition is for filtering while -SkipCompatibilityCheck is for enforcement.

These test modules are pretty basic. I wonder if it's better to just generate these at runtime as test variations?

Seems overkill since it doesn't appear the tests need to push more than once before a pop. Existing tests simply store currentPSModulePath in Before and restore in After

Perhaps else is more readable.

Perhaps else is more readable.

We use compatiblePSEditions ?? DefaultCompatiblePSEditions twice. Maybe use a variable?

Should we add defaults too?

My thinking is that the PSModuleInfo should reflect the manifest. But I think this particular point needs further discussion, because rewriting default logic everywhere is error prone. I'm reluctant to add another public field on to the PSModuleInfo (and that would probably merit an RFC anyway).

But so far, the way it works is:

• PSModuleInfo.CompatiblePSEditions is exactly what's in the manifest (or an empty array)
• Everywhere we check CompatiblePSEditions a default is used if it's null or empty

I meant that if compatiblePSEditions == null we should add default.

Yeah, I'm thinking that we shouldn't.

If we do, then we are not reflecting what the manifest says. So there will be no way to differentiate between a manifest that has CompatiblePSEditions = @("Desktop") and no CompatiblePSEditions field at all -- all our tools to parse a manifest will incorrectly see them as the same.

For the purposes of this logic, yes we assume a default. But this is quite specific logic. So I think we need to preserve the information for other functionalities that may need to distinguish the scenarios.

But maybe we also need a way to record the default assumed value on the PSModuleInfo?

I agree with @rjmholt that the member should reflect what is actually in the manifest.

Can we use Path.Combine()?

This is to do "C:\path\one;C:\path\two" + "C:\path\three"; we're not combining paths into one path, but concatenating multiple paths into the PSModulePath using a path separator (';'). Is there a method for that?

No, sorry - I skipped this.

#Closed.

Please remove the unneeded comment.

#Closed.

We can place the comment in one line.

#Closed.

We can place the comment in one line.

#Closed.

Should we use imperative mood in Use 'Import-Module -SkipCompatibilityCheck'?
Also {0} can be long path.

We should use HasFlag() - now it is optimized.

Oh didn't see this before! Good idea!

Maybe 9?

I wonder that we move the block - have we I side effect in lines 3450-3451?

I wonder that we move the block - have we I side effect in lines 3450-3451?

The if block? My goal is for the PSModuleInfo to accurately reflect the module manifest being loaded. It seems to be working for our purposes at present -- I moved the block down because I realised isOnSystem32ModulePath wasn't always set when a module was actually found there.

AppVeyor is failing this test, but it's passing on my machine. Trying the command itself gives me the right exception. Stepping through the code in the debugger does what I expect. Any ideas?

Why we use SilentlyContinue not Stop?

What is benefits from moving the check on up level?

Please remove first space before All.

The same. What is benefits from moving the check on up level?

The callee assertion seems to be a common occurrence in the codebase -- I could have included a comment, but thought an assertion was better as "zero-cost executable documentation".

Putting the actual if-check inside the method means needing to have a separate for-loop to handle the unchecked enumeration (since we can't use both return and yield return in the same method). Which doubles the size of the method and I think makes it less readable too. Doing it like this, we can avoid the whole method call (and in the case of the condition not being met, an unneeded layer of indirection in the enumeration) ahead of time and keep the method itself cohesive.

return -> yield break?

we can avoid the whole method call

I think a performance is not so important here.

Well yield break will return an empty enumeration. But if PSEdition is not set you don't want that, you want everything. So you need another for-loop just for the case where it's not set, which seems pretty ugly to me compared to just opting out of the filter.

Yes I very much agree that the performance gain is minimal and not needed. Just pointing out a separate virtue of the approach I took.

I think the # if UNIX part can be abstracted in IsPSEditionCompatible, and then code here don't need to be divided up.
The IsPSEditionCompatible would look like this:

        private bool IsPSEditionCompatible(
            string moduleManifestPath,
            IEnumerable<string> compatiblePSEditions,
            out bool isOnSystem32ModulePath)
        {
            isOnSystem32ModulePath = false;
#if UNIX
            return false;
#else
            string windowsPSModulePath = ModuleIntrinsics.GetWindowsPowerShellPSHomeModulePath();
            if (!moduleManifestPath.StartsWith(windowsPSModulePath, StringComparison.OrdinalIgnoreCase))
            {
                return true;
            }

            isOnSystem32ModulePath = true;

            return BaseSkipEditionCheck || Utils.IsPSEditionSupported(compatiblePSEditions);
#endif
        }

Yes I thought I'd start with the "zero runtime overhead on UNIX" approach and see how everyone felt. I definitely think your way is cleaner!

It's better to have a static field keeping the value, so we only need to calculate it once.

That's what I wanted to do. I assume the environment variable can't change then?

System.Environment.SystemDirectory doesn't use the environment variable. It calls the win32 API GetSystemDirectoryW. See https://msdn.microsoft.com/en-us/library/windows/desktop/ms724373(v=vs.85).aspx

Oh didn't see this before! Good idea!

The if block? My goal is for the PSModuleInfo to accurately reflect the module manifest being loaded. It seems to be working for our purposes at present -- I moved the block down because I realised isOnSystem32ModulePath wasn't always set when a module was actually found there.

I quite dislike this -- not sure if there's a better way to do it?

I see we have private IEnumerable<PSModuleInfo> GetModuleForRootedPaths(string[] modulePaths, bool all, bool refresh)

Going to change this over to the Utils method and optimise that one slightly. Want to let @daxian-dbw finish his review first (GitHub has had a nasty habit of forcing me to rewrite comments when new commits are added recently...)

Why we use IEnumerable? We could use List - seems it is more thriftily.

Used IEnumerable here because:

• The interface type defines all the functionality we need without added constraints
• We don't require indexing or even size checking, just ordered enumeration
• The method is free to implement the collection as it wishes

But, perhaps there are benefits to using a List or at least an IList?

• Less type complexity (and binding to a concrete type not an issue with an internal API)?
• Provides strictness and ordering guarantees

I see IEnumerable benefits:

• it allows to implement generic algorithms. Seems we don't need this.
• it allows to postpone the execution (and implement interruption of long excution). Seems we don't need this too.

I believe we get more simple (readable and debuggable) code with List or Collection.

I see we have private IEnumerable<PSModuleInfo> GetModuleForRootedPaths(string[] modulePaths, bool all, bool refresh)

The file name doesn't contain Module. Maybe ModuleCompatiblePSEditions.Tests.ps1?