New-MarkdownHelp implementation for v2

[adityapatwardhan]

• Move the v1 code to sub-folder
• Create structure for v2
• Implement New-MarkdownHelp
• Add xunit tests
• Add Pester tests
• Add new build.ps1

[daxian-dbw]

The use of nullable-reference is a little confusing, and some seem wrong as #nullable enable is not added to all the new files. I suggest to apply #nullable enable to all new files, and that will help you get compiler's help to see if any use of nullable-reference is correct.

[adityapatwardhan]

@daxian-dbw This is ready for re-review. I have removed the StringBuilder pool implementation as it was causing high memory consumption issues. I will include it in separate PR.

[daxian-dbw]

@adityapatwardhan Don't copy the exact ObjectPool implementation from https://docs.microsoft.com/en-us/dotnet/standard/collections/thread-safe/how-to-create-an-object-pool:

1. you don't need a generic implementation, so no need to have ObjectPool<T> and pass in the object generator.
2. you should borrow the Create and Return implementation of StringBuilderPooledObjectPolicy, defining capacity of the pool and a new string builder. But most importantly, you need to clear the StringBuilder instance when returning it.

You ran into a memory issue because you didn't clear the StringBuilder instance when returning.

BTW, I don't recommend to use the new() syntax. For a type with default constructor, new() makes it hard to search where the default constructor is called for this type. Take PipelineProcessor in PowerShell as an example, it has a default constructor, so the only way to look for calls to the default constructor is by searching new PipelineProcessor(). Searching all reference to PipelineProcessor type is not helpful in our case because there are too many references to it. If you use new() extensively, it would be impossible to do that search.

It might be fine to use this syntax in a small size code base, but I don't recommend to use it in the PowerShell repo.

[adityapatwardhan]

Thanks for the feedback. Do you think ObjectPool change is needed in this PR or can be brought in later?

[daxian-dbw]

Since we have been talking about it, and the changes to correct the memory issue is relatively simple, let's maybe get it done in this PR?

[daxian-dbw]

@adityapatwardhan But not trying to push it. I'm totally fine if you want to do it in a separate PR. So it's up to you 😄

[adityapatwardhan]

@daxian-dbw you convinced me against the usage of new() :)

[daxian-dbw]

@adityapatwardhan You can absolutely use it when it's appropriate, such as for initializing the file/property members. But just be cautious when using it in other places 😄

[adityapatwardhan]

@daxian-dbw It is ready for review.

[iSazonov]

.Net team announced End Of Support for Net461. It seems time to move from net461 to net462.

[daxian-dbw]

I wish we can move directly to netstandard2.0, which implies that we support .NET Framework 4.7.2+ (.NET versions prior to 4.7.2 did NOT ship the netstanardlib.dll and thus doesn't work well with netstandard2.0 applications).

[adityapatwardhan]

I will make that change in a different PR. Needs to be tested in older supported OSes. I can change to 4.6.2 now, since it is a revision update.

[daxian-dbw]

@adityapatwardhan No need to do that now. The end of support is on April 26, 2022

[adityapatwardhan]

@daxian-dbw Addressed your comments. Keeping it net461 and hoping to move directly to netstandard2.0 in the future.

[daxian-dbw]

It's recommended to use a separate type for the IModuleAssemblyCleanup logic, with a name that is easy to be recognized for its functionality, like "ModuleCleanUp".

[adityapatwardhan]

yes, makes sense since it is for cleaning up the module, not just a cmdlet.

[daxian-dbw]

Do you expect CommandHelpMarkdownWriter to be inherited by another type? If not, then add sealed to the type declaration, and then just move the logic in Dispose(bool disposing) here and remove the GC.SuppressFinalize call. You don't even need the disposedValue field unless you are checking it somewhere else in the code and will throw a ObjectDisposedException.

[adityapatwardhan]

fixed.

[daxian-dbw]

Same here.

[adityapatwardhan]

fixed

[daxian-dbw]

LGTM