Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

Add new opt-in common test for markdown link linting #273

Merged
merged 8 commits into from
Aug 17, 2018

Conversation

johlju
Copy link
Contributor

@johlju johlju commented Aug 7, 2018

This partly resolves the issue #211.


This change is Reviewable

@review-me review-me bot added the needs review The pull request needs a code review. label Aug 7, 2018
@codecov-io
Copy link

codecov-io commented Aug 7, 2018

Codecov Report

Merging #273 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #273   +/-   ##
===================================
  Coverage    73%    73%           
===================================
  Files        12     12           
  Lines      1543   1543           
  Branches      2      2           
===================================
  Hits       1129   1129           
  Misses      412    412           
  Partials      2      2

@johlju
Copy link
Contributor Author

johlju commented Aug 7, 2018

@review-me review-me bot removed the needs review The pull request needs a code review. label Aug 7, 2018
@johlju
Copy link
Contributor Author

johlju commented Aug 7, 2018

@PlagueHO Another one to review if you have time. No hurry. 🙂

Copy link
Contributor Author

@johlju johlju left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion


README.md, line 247 at r2 (raw file):

* **Common Tests - Custom Script Analyzer Rules**: fail tests if any
  custom script analyzer rules are violated.
* **Common Tests - Validate Markdown Links**: fails tests if a link in

This common test should be documented a bit more, for example using absolute paths might not always work.
E.g if having a \Examples\README.md that contaon an absolute link /Examples/Resources/SqlAG will fail, but a relative link Resources/SqlAG will pass (issue vors/MarkdownLinkCheck#5).

@johlju johlju force-pushed the add-markdown-link-linting branch 4 times, most recently from d0d98ea to 40dd02c Compare August 15, 2018 10:57
Copy link
Contributor

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

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

Awesome stuff - can't wait to opt-in to this one too. Just some minor bits.

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @PlagueHO and @johlju)


Meta.Tests.ps1, line 1013 at r3 (raw file):

                if (-not (Get-Module -Name $dependencyModuleName -ListAvailable))
                {
                    # Remember that we installed the module, so it get uninstalled.

so it get -> so that it gets


Meta.Tests.ps1, line 1025 at r3 (raw file):

    $markdownFileExtensions = @('.md')

    $markdownFiles = (Get-TextFilesList $moduleRootFilePath) |

Do we need braces ( ) around this?


Meta.Tests.ps1, line 1026 at r3 (raw file):

    $markdownFiles = (Get-TextFilesList $moduleRootFilePath) |
        Where-Object { $markdownFileExtensions -contains $_.Extension }

Should be named parameter -FilterScript and could we also put script block on next line?


Meta.Tests.ps1, line 1041 at r3 (raw file):

                # Make sure it always returns an array even if Get-MarkdownLink returns $null.
                $brokenLinks = @(Get-MarkdownLink @getMarkdownLinkParameters)
                if ($brokenLinks.Count -gt 0)

Can you add a blank line above the if statement?


Meta.Tests.ps1, line 1063 at r3 (raw file):

                    Remove-Module -Name $dependencyModuleName -Force

                    Remove-Item -Path "$HOME\Documents\WindowsPowerShell\Modules\$dependencyModuleName" -Recurse -Force

Why not use Uninstall-Module here?


Meta.Tests.ps1, line 1066 at r3 (raw file):

                    if ((Get-Module -Name $dependencyModuleName -ListAvailable))
                    {
                        throw 'Dependency module was not uninstall correctly.'

uninstall -> uninstalled


README.md, line 247 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This common test should be documented a bit more, for example using absolute paths might not always work.
E.g if having a \Examples\README.md that contaon an absolute link /Examples/Resources/SqlAG will fail, but a relative link Resources/SqlAG will pass (issue vors/MarkdownLinkCheck#5).

Do we want to add some documentation into the DSCResources StyleGuidelines.md/BestPractices.md to specify how paths should be formatted?


README.md, line 292 at r3 (raw file):

  custom script analyzer rules are violated.
- **Common Tests - Validate Markdown Links**: fails tests if a link in
  a markdown file are broken.

are -> is

@johlju johlju force-pushed the add-markdown-link-linting branch from 40dd02c to f941a04 Compare August 16, 2018 08:04
Copy link
Contributor Author

@johlju johlju left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 8 unresolved discussions (waiting on @PlagueHO and @johlju)


Meta.Tests.ps1, line 1013 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

so it get -> so that it gets

Done.


Meta.Tests.ps1, line 1025 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Do we need braces ( ) around this?

Done. Nope, must have done more inside them and the refactored and forgot the parenthesis. :)


Meta.Tests.ps1, line 1026 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should be named parameter -FilterScript and could we also put script block on next line?

Done.


Meta.Tests.ps1, line 1041 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a blank line above the if statement?

Done.


Meta.Tests.ps1, line 1063 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Why not use Uninstall-Module here?

Done. Well, embarrassing... I didn't know that cmdlet existed. 😅


Meta.Tests.ps1, line 1066 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

uninstall -> uninstalled

Done. The code was refactored per previous comment.


README.md, line 247 at r2 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Do we want to add some documentation into the DSCResources StyleGuidelines.md/BestPractices.md to specify how paths should be formatted?

It's a bug I think, since it is a valid path, the linter just did not recognize it. So any valid markdown path should work, not sure we should duplicate that documentation? I added a note about it in the README.md here for now. Is it good enough you think?
Bit I'm open to add a section to the bestpractices.md too.

We should add short explainations like this for the other common tests too I think. Started with one of other tests in a PR (the "spelling" one).


README.md, line 292 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

are -> is

Done.

Copy link
Contributor

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @johlju)


Meta.Tests.ps1, line 1063 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Done. Well, embarrassing... I didn't know that cmdlet existed. 😅

Too many cmdlets to know them all! 😁


README.md, line 247 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

It's a bug I think, since it is a valid path, the linter just did not recognize it. So any valid markdown path should work, not sure we should duplicate that documentation? I added a note about it in the README.md here for now. Is it good enough you think?
Bit I'm open to add a section to the bestpractices.md too.

We should add short explainations like this for the other common tests too I think. Started with one of other tests in a PR (the "spelling" one).

Agreed - better to not duplicate and I think adding the section to the README.MD here is great!


README.md, line 300 at r4 (raw file):

will pass the linter.

>**NOTE!** There are currently a bug in the markdown link linter that makes it

There are currently -> this is currently


README.md, line 301 at r4 (raw file):

>**NOTE!** There are currently a bug in the markdown link linter that makes it
>unable to recognized absolute paths where the absolute link starts in a parent

recognized -> recognize


README.md, line 303 at r4 (raw file):

>unable to recognized absolute paths where the absolute link starts in a parent
>folder.
>For example, if having a markdown file in `/Examples/README.md`,

My read better as: For example, if a markdown file /Example/README.md contains an absolute link pointing to...

Copy link
Contributor Author

@johlju johlju left a comment

Choose a reason for hiding this comment

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

Got them all now I hope. 😃

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @PlagueHO)


README.md, line 300 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

There are currently -> this is currently

Done.


README.md, line 301 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

recognized -> recognize

Done.


README.md, line 303 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

My read better as: For example, if a markdown file /Example/README.md contains an absolute link pointing to...

Done.

@johlju johlju force-pushed the add-markdown-link-linting branch from 1442a80 to aecd309 Compare August 17, 2018 05:57
Copy link
Contributor

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@johlju johlju merged commit a0c7e86 into PowerShell:dev Aug 17, 2018
@johlju johlju removed the needs review The pull request needs a code review. label Aug 17, 2018
@johlju
Copy link
Contributor Author

johlju commented Aug 17, 2018

@PlagueHO Thank you! Awesome reviews as usually! 😃

@johlju johlju deleted the add-markdown-link-linting branch August 17, 2018 07:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants