search

PowerShell / PSDscResources

MIT License
129 stars 53 forks source link

Opt-In to Meta Test - Fixes #172 #173

Closed PlagueHO closed 3 years ago

PlagueHO commented 5 years ago

Pull Request (PR) description

This PR opts in to Meta Tests and resolves any issue identified.

This Pull Request (PR) fixes the following issues

Task list

@mhendric - could you review this one for me when you get time?

This change is Reviewable

codecov-io commented 5 years ago

Codecov Report

Merging #173 into dev will not change coverage. The diff coverage is 33%.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #173   +/-   ##
===================================
  Coverage    83%    83%           
===================================
  Files        19     19           
  Lines      2760   2760           
  Branches      4      4           
===================================
  Hits       2303   2303           
  Misses      453    453           
  Partials      4      4
mhendric commented 4 years ago

Hey @PlagueHO. Looks like you got a CI failure. Ping me once this is ready for a final review. Thanks.

Custom DSC Resource Kit PSSA rule(s) did not pass. 971 The following PSScriptAnalyzer rule 'DscResource.AnalyzerRules\Measure-Keyword' errors need to be fixed: 972 GroupSet.schema.psm1 (Line 35): 'Configuration' statements should not contain upper case letters See https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-keywords 973 For instructions on how to run PSScriptAnalyzer on your own machine, please go to https://github.com/powershell/PSScriptAnalyzer 974 [-] Should pass all custom DSC Resource Kit PSSA rules 808ms 975 Expected $null, but got Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticRecord. 976 582: $customPssaRulesOutput | Should -Be $null 977 at , C:\projects\psdscresources\DscResource.Tests\Meta.Tests.ps1: line 582

PlagueHO commented 4 years ago

@johlju - the current DSCResource.Tests require a lower case 'c' to be used for the Configuration keyword - this makes sense because it is a keyword and we've enforced lower case letters for all keywords.

However, looking through most repos, we're not doing this (e.g. see Examples).

Two things jump to mind here:

  1. We need to define if we are including Configuration as a keyword that should be all lower case.
  2. We need to apply our code style tests to examples.

What is your opinion on this? I'd prefer to use lower case 'c' for configuration for standardization- but it will have an impact if we extend style tests to examples.

cc @SSvilen

PlagueHO commented 4 years ago

Hi @mhendric - we added a bunch of code style tests a while back that validate all keywords are all lower case. As "Configuration" is a keyword it is being caught in this test. Only just encountered this because most other repos don't have "Configuration" in the modules folder - and we're not scanning the Examples or Tests for code style (the other places "Configuration" is used). So we'll need to decide whether we want to eliminate "Configuration" from the list of keywords.

My preference would be standardization: All keywords all lower case (makes it easier to remember ;) ).

johlju commented 4 years ago

I had the same problem in another module. I just change the code to use lower-case ‘c’ since I also thought it best to have one standard, all keywords lower-case. I think we should change all the configuration blocks to use lower-case ‘c’.

mhendric commented 4 years ago

Hey Guys, I'm a bit concerned that if we mandate a casing for this that is not the default casing, we're going to be hitting this issue anytime someone's IDE (correctly) autocompletes 'configuration' to 'Configuration'. Not to mention every DSC module is going to have to be updated to use an incorrect case for this keyword. Thoughts?

mhendric commented 4 years ago

@SSvilen , good catch. That wasn't exactly what I was getting at, but I think you are right that Configuration is DSC specific. Further, our official documentation of it shows it with a capital C:

https://docs.microsoft.com/en-us/powershell/scripting/dsc/configurations/configurations?view=powershell-6

mhendric commented 4 years ago

Looks like another related DSC keyword is 'Node'. At least in the PowerShell ISE, that keyword autocompletes with a capital N as well.

SSvilen commented 4 years ago

If everyone agrees I can remove all DSC related keywords from the check.

johlju commented 4 years ago

@SSvilen lets make an exception for the keywords Configuration and Node. They will autocomplete in VS Code, but not the keyword function for example. So those two keywords can be written with first letter as lower-case or upper-case. If possible add an array with these two keywords if there are more in the future we need to make an exception for.

It will be less code to change, and we can change that to also have lower-case in the future.

/cc @PlaugeHO

PlagueHO commented 4 years ago

@SSvilen - :+1: from me - sorry for taking so long to get back. Will be a bit slow to respond for the next few weeks but will try and keep on top of things.

SSvilen commented 4 years ago

The PR has been submitted.