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

Adding opt-in common test for spellchecking markdown files #279

Merged
merged 12 commits into from
Aug 31, 2018

Conversation

johlju
Copy link
Contributor

@johlju johlju commented Aug 10, 2018

Fixes #211
Fixes #281

Worked on this last evening, there are a bug with this, but sending this in so it is not forgotten in my fork.


This change is Reviewable

@johlju johlju added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Aug 10, 2018
@review-me review-me bot added the needs review The pull request needs a code review. label Aug 10, 2018
@codecov-io
Copy link

codecov-io commented Aug 10, 2018

Codecov Report

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

Impacted file tree graph

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

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

johlju commented Aug 10, 2018

Tested OK here:
CertificateDsc (Not opt-in): https://ci.appveyor.com/project/johlju/certificatedsc/build/2.3.40.0#L460
CertificateDsc (Opt-in and failing test): https://ci.appveyor.com/project/johlju/certificatedsc/build/2.3.39.0#L465

@johlju
Copy link
Contributor Author

johlju commented Aug 10, 2018

Test FAILED here Test passes, see next comment, although with a workaround for the bug in cSpell:
CertificateDsc (opt-in, and passing tests): https://ci.appveyor.com/project/johlju/certificatedsc/build/2.3.45.0#L460

This problem has been reported here: streetsidesoftware/cspell#61

It is using this cSpell configuration file (.vscode/cSpell.json):

{
    "ignorePaths": [
        ".git/*",
        ".vscode/*",
        "DSCResource.Tests/*",
        "DscResource.Tests/*"
    ],
    "language": "en",
    "dictionaries": [
        "powershell"
    ],
    "words": [
        "FIPS",
        "MSFT",
        "HQRM",
        "markdownlint",
        "Chocolatey",
        "codecov"

    ],
    "ignoreRegExpList": [
        "\\.gitignore",
        "\\.gitattributes",
        "AppVeyor",
        "[email protected]"
    ]
}

The key ignorePaths, that is because 'DscResource.Tests` are written twice (locally when cloned it has upper-case in 'Dsc', issue PowerShell/DscResources#431).

The key dictionaries might not be necessary for this test to pass, but good to have when using the VSCode extension Code Spell Checker.

This configuration file, and how it can be used by the test and VS Code should be documented before merge.

@johlju
Copy link
Contributor Author

johlju commented Aug 10, 2018

@johlju johlju force-pushed the add-markdown-spell-check branch from 2a02339 to fa6cc37 Compare August 10, 2018 16:26
@johlju johlju changed the title WIP: Adding opt-in common test for spellchecking markdown files Adding opt-in common test for spellchecking markdown files Aug 10, 2018
@johlju johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Aug 10, 2018
@johlju johlju force-pushed the add-markdown-spell-check branch 3 times, most recently from cdb92b6 to 307ed32 Compare August 15, 2018 06:50
@johlju
Copy link
Contributor Author

johlju commented Aug 15, 2018

Need help review this PR. I anyone wants to review this PR, or any other PR, you are more than welcome! Love to have more people helping out with reviews!

@johlju johlju force-pushed the add-markdown-spell-check branch 2 times, most recently from d4cc5ff to 65d9eb0 Compare August 15, 2018 10:55
@johlju
Copy link
Contributor Author

johlju commented Aug 16, 2018

I will rebase once the other two PR's are through, because they made changes in sections in markdown that was moved in this PR. But anyone, please start the review anyway.

johlju added 8 commits August 17, 2018 09:07
- Adding opt-in common test for spellchecking markdown files. Opt-in by
  adding "Common Tests - Spellcheck Markdown Files" in the file
  .MetaTestOptIn.json (issue PowerShell#211).
…dded the

  settings file `.vscode\cSpell.json`.
- Move section Phased Meta test Opt-In in the README.md,
  and renamed it to Common Meta test Opt-In (issue PowerShell#281).
@johlju johlju force-pushed the add-markdown-spell-check branch from 65d9eb0 to 279a477 Compare August 17, 2018 07:12
@PlagueHO
Copy link
Contributor

Cool. Let me know when and I'll review.

@johlju
Copy link
Contributor Author

johlju commented Aug 17, 2018

@PlagueHO It's ready now - the tests should pass in a few minutes. Thank you!

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 4 of 4 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @johlju)


Meta.Tests.ps1, line 1085 at r1 (raw file):

        It 'Should not have spelling errors in any markdown files' -Skip:(!$optIn) {
            $spellcheckSettingsFilePath = Join-Path -Path $repoRootPath -ChildPath '.vscode\cSpell.json'
            if (Test-Path -Path $spellcheckSettingsFilePath)

Can add a blank line above here?


Meta.Tests.ps1, line 1150 at r1 (raw file):

        if (Test-Path $errorFileName)
        {
            Remove-Item -Path $errorFileName -Force

FYI, Could assign to $null so that we don't end up outputting result to output stream.


README.md, line 123 at r1 (raw file):

#### Common Tests - Spellcheck Markdown Files

When opt-in for this test, if there are any spelling errors in markdown files,

When opt-in for -> When opt-in to


README.md, line 127 at r1 (raw file):

>**Note:** The spell checker is case-insensitive, so the words 'AppVeyor' and
>'appveyor' is equal and both allowed.

is equal and both allowed. -> are equal and both are allowed.


README.md, line 130 at r1 (raw file):

If the spell checker ([cSpell](https://www.npmjs.com/package/cspell)) does not
recognize the word, but the word are correct, or maybe there are a specific phrase

Might read better as:

If the spell checker ([cSpell](https://www.npmjs.com/package/cspell)) does not
recognize the word, but the word is correct or a specific phrase is not recognized
but should be allowed, then it is possible to add these to a dictionary or tell it to
ignore the word of phrases. This is done by adding a `\.vscode\cSpell.json` in
the repository.

README.md, line 134 at r1 (raw file):

tell it to ignore words or phrases.

By adding a file `\.vscode\cSpell.json` in the repository, the spell checker

This line could be removed if using the text above.


README.md, line 137 at r1 (raw file):

will follow the settings in this file.

The simplest form of the file `\.vscode\cSpell.json` is this (see

The following JSON is the simplest form of the file \.vscode\cSpell.json (see cSpell for more settings).


README.md, line 174 at r1 (raw file):

The key `ignoreRegExpList` is better to use to ignore phrases or combination of

The key ignoreRegExpList is used to ignore phrases or combinations of words, such as AppVeyor, which will be detected as two different words since it consists of two words starting with upper-case letters.


README.md, line 177 at r1 (raw file):

words, like 'AppVeyor', it will detect that word as two different words, since
it consist of two words with upper-case letter.
So for it to ignore 'AppVeyor', as we know it's correct, we can add a regular

To configure cSpell to ignore AppVeyor, then we can add a regular expression, in this case AppVeyor. This will cause cSpell to ignore part of the text that matches the regular expression.

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: 2 of 4 files reviewed, 8 unresolved discussions (waiting on @PlagueHO)


Meta.Tests.ps1, line 1085 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can add a blank line above here?

Done.


README.md, line 123 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

When opt-in for -> When opt-in to

Done.


README.md, line 127 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

is equal and both allowed. -> are equal and both are allowed.

Done.


README.md, line 130 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Might read better as:

If the spell checker ([cSpell](https://www.npmjs.com/package/cspell)) does not
recognize the word, but the word is correct or a specific phrase is not recognized
but should be allowed, then it is possible to add these to a dictionary or tell it to
ignore the word of phrases. This is done by adding a `\.vscode\cSpell.json` in
the repository.

Done. Thank you for the updated text in the review comments! Much better now! 😄


README.md, line 134 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This line could be removed if using the text above.

Done.


README.md, line 137 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

The following JSON is the simplest form of the file \.vscode\cSpell.json (see cSpell for more settings).

Done.


README.md, line 174 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
The key `ignoreRegExpList` is better to use to ignore phrases or combination of

The key ignoreRegExpList is used to ignore phrases or combinations of words, such as AppVeyor, which will be detected as two different words since it consists of two words starting with upper-case letters.

Done.


README.md, line 177 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

To configure cSpell to ignore AppVeyor, then we can add a regular expression, in this case AppVeyor. This will cause cSpell to ignore part of the text that matches the regular expression.

Done.

@johlju
Copy link
Contributor Author

johlju commented Aug 29, 2018

@PlagueHO when you have a chance, can you see if I got all the review comments, and there are no new comments? 🙂

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.

Almost perfect @johlju - but I had one mistake in my suggested text that needs to be corrected.

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @PlagueHO)


README.md, line 130 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Done. Thank you for the updated text in the review comments! Much better now! 😄

Doh - slight mistake (from my text): word of phrases -> word or phrases

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: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @PlagueHO)


README.md, line 130 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Doh - slight mistake (from my text): word of phrases -> word or phrases

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.

:lgtm:

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

@PlagueHO PlagueHO merged commit e8bf199 into PowerShell:dev Aug 31, 2018
@johlju johlju removed the needs review The pull request needs a code review. label Aug 31, 2018
@johlju johlju deleted the add-markdown-spell-check branch January 5, 2019 11:17
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