Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve checking of module markdown files in GetMarkdownFilesFromPath #456

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bmsimons
Copy link

Fixes #447

@adityapatwardhan
Copy link
Member

@bmsimons Can you add tests for this?

@adityapatwardhan adityapatwardhan requested review from vors and adityapatwardhan and removed request for vors April 22, 2019 18:20
[https://test.com](https://test.com)


'@
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the "setup" steps in this It should be moved to BeforeAll, especially because the files are used in another It.

Copy link
Collaborator

@vors vors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking a stub at it!

Please include some text explanation of the problem that tries to solve and how it's solved in the PR main body. At the moment it just reference the bug number. It requires additional hopes (and web browser) to figure out the context. After few years, it would be much harder to do - it's good idea to include the minimal context in the PR body, so when it's merged it will be preserved in git history directly.

$MarkdownFilesFiltered = $MarkdownFiles
} Else {
ForEach ($File in $MarkdownFiles) {
$Matches = [regex]::Match((Get-Content $File.FullName -Raw), "# $($File.BaseName) Module").Captures.Groups
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand that line, can you add a comment that explains the intention here?

@@ -1512,7 +1512,20 @@ function GetMarkdownFilesFromPath
}
}

return $MarkdownFiles
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going this route of the more intelligent heuristic, we should remove the old dash-based heuristic.


It "Checks if external help files are generated correctly with dash in module markdown file" {
Rename-Item -Path (Join-Path $TestDrive "\docs\TestModule.md") -NewName "Test-Module.md"
(Get-Content "$TestDrive\docs\Test-Module.md") -replace "TestModule", "Test-Module" | Set-Content "$TestDrive\docs\Test-Module.md"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it needed, why we cannot create it directly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New-ExternalHelp fails if Module markdown file has a dash at 'ModuleName Cmdlets'
4 participants