This repository was archived by the owner on Feb 24, 2021. It is now read-only.
Hashtables - correct formatting check #348
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## dev #348 +/- ##
====================================
+ Coverage 80% 80% +<1%
====================================
Files 12 12
Lines 1787 1805 +18
Branches 2 2
====================================
+ Hits 1431 1447 +16
- Misses 354 356 +2
Partials 2 2 |
|
@SSvilen Awesome work on this one! Thank you! This one will probably generate a lot of issues that need to be resolved in the modules. 🙂 |
|
@PlagueHO Do you have time to review this one? |
|
@johlju - yep, will get onto this one today. |
PlagueHO
suggested changes
Sep 21, 2019
There was a problem hiding this comment.
Great stuff @svilen - just some minor style issues.
I'll also run your branch against all the repos to see what shows up.
Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @SSvilen)
DscResource.AnalyzerRules/DscResource.AnalyzerRules.psm1, line 1056 at r1 (raw file):
<# .SYNOPSIS Validates all hashtables.
Can you indent this line (and the other help section content lines)?
DscResource.AnalyzerRules/DscResource.AnalyzerRules.psm1, line 1077 at r1 (raw file):
[CmdletBinding()] [OutputType([Microsoft.Windows.Powershell.ScriptAnalyzer.Generic.DiagnosticRecord[]])] param(
LOL - looks like you need to add a space after the param 😁
DscResource.AnalyzerRules/DscResource.AnalyzerRules.psm1, line 1082 at r1 (raw file):
[System.Management.Automation.Language.HashtableAst[]] $HashtableAst )
Can you add a blank line after this one?
DscResource.AnalyzerRules/DscResource.AnalyzerRules.psm1, line 1091 at r1 (raw file):
$hashtableLines = $hashtable.Extent.Text -split '\n' #hashtable should start with '@{' and end with '}'
Can you add a space after the # and start comment with a capital letter?
DscResource.AnalyzerRules/DscResource.AnalyzerRules.psm1, line 1103 at r1 (raw file):
$initialIndent = ([regex]::Match($hashtable.Extent.StartScriptPosition.Line, '(\s*)')).Length $expectedLineIndent = $initialIndent + 5 foreach ($keyValuePair in $hashtable.KeyValuePairs)
Can you add a blank line before this one?
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3377 at r1 (raw file):
Describe 'Measure-Hashtable' {
Can you remove this blank line?
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3378 at r1 (raw file):
Describe 'Measure-Hashtable' { Context "When calling the function directly" {
Can you convert to single quotes?
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3428 at r1 (raw file):
} Context "When hashtable is correctly formatted" {
Can you use single quotes?
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3429 at r1 (raw file):
Context "When hashtable is correctly formatted" { It "Correctly formatted non-nested hashtable" {
Can you use single quotes?
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3441 at r1 (raw file):
$record = Measure-Hashtable -HashtableAst $mockAst ($record | Measure-Object).Count | Should -Be 0 }
Can you add a blank line after this one?
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3442 at r1 (raw file):
($record | Measure-Object).Count | Should -Be 0 } It "Correctly formatted nested hashtable" {
Can you use single quotes?
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3461 at r1 (raw file):
} Context "When calling PSScriptAnalyzer" {
Can you use single quotes?
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3467 at r1 (raw file):
} $ruleName = "$($script:ModuleName)\Measure-Hashtable" }
Can you add a blank line after this one?
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3468 at r1 (raw file):
$ruleName = "$($script:ModuleName)\Measure-Hashtable" } Context 'When hashtable is not correctly formatted' {
Indent seems incorrect here.
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3509 at r1 (raw file):
} Context "When hashtable is correctly formatted" {
Can you use single quotes?
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3510 at r1 (raw file):
Context "When hashtable is correctly formatted" { It "Correctly formatted non-nested hashtable" {
Can you use single quotes?
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3521 at r1 (raw file):
$record = Invoke-ScriptAnalyzer @invokeScriptAnalyzerParameters ($record | Measure-Object).Count | Should -Be 0 }
Can you add a blank line after this one?
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3522 at r1 (raw file):
($record | Measure-Object).Count | Should -Be 0 } It "Correctly formatted nested hashtable" {
Can you use single quotes?
PlagueHO
reviewed
Sep 21, 2019
There was a problem hiding this comment.
Just running against the repo and it looks like there is a false positive showing up. Empty hashtables are showing as violations. E.g. https://github.com/PowerShell/ActiveDirectoryCSDsc/blob/dev/DSCResources/MSFT_AdcsCertificationAuthority/MSFT_AdcsCertificationAuthority.psm1#L207
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @SSvilen)
|
FYI. Sent in PR PowerShell/DscResources#531 to update the style guideline so it is more clear that we write |
SSvilen
commented
Sep 22, 2019
There was a problem hiding this comment.
OK I'll update the check's logic.
Reviewable status: 3 of 6 files reviewed, 18 unresolved discussions (waiting on @PlagueHO)
DscResource.AnalyzerRules/DscResource.AnalyzerRules.psm1, line 1056 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you indent this line (and the other help section content lines)?
Done.
DscResource.AnalyzerRules/DscResource.AnalyzerRules.psm1, line 1077 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
LOL - looks like you need to add a space after the param 😁
Thank god there is a check for that :) are the meta tests turned on for this repo? I branched out of dev, where this check is not yet merged.
DscResource.AnalyzerRules/DscResource.AnalyzerRules.psm1, line 1082 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you add a blank line after this one?
Done.
DscResource.AnalyzerRules/DscResource.AnalyzerRules.psm1, line 1091 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you add a space after the # and start comment with a capital letter?
Done.
DscResource.AnalyzerRules/DscResource.AnalyzerRules.psm1, line 1103 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you add a blank line before this one?
Done.
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3377 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you remove this blank line?
Done.
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3378 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you convert to single quotes?
Done.
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3428 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use single quotes?
Done.
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3429 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use single quotes?
Done.
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3441 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you add a blank line after this one?
Done.
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3442 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use single quotes?
Done.
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3461 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use single quotes?
Done.
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3467 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you add a blank line after this one?
Done.
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3468 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Indent seems incorrect here.
Looks fine to me.
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3509 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use single quotes?
Done.
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3510 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use single quotes?
Done.
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3521 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you add a blank line after this one?
Done.
Tests/Unit/DscResource.AnalyzerRules.Tests.ps1, line 3522 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use single quotes?
Done.
SSvilen
force-pushed
the
HashtableCheck
branch
from
0e308d6 to
825c4a0
Compare
SSvilen
commented
Sep 23, 2019
There was a problem hiding this comment.
That's just a rebase against the current dev branch.
Reviewable status: 1 of 6 files reviewed, 18 unresolved discussions (waiting on @PlagueHO)
PlagueHO
suggested changes
Sep 26, 2019
There was a problem hiding this comment.
Changes looking great. I'll just run this against all the repos and see what shows up!
Reviewed 1 of 3 files at r2, 4 of 4 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SSvilen)
DscResource.AnalyzerRules/DscResource.AnalyzerRules.psm1, line 1077 at r1 (raw file):
Previously, SSvilen (Svilen) wrote…
Thank god there is a check for that :) are the meta tests turned on for this repo? I branched out of dev, where this check is not yet merged.
That is a good point - they should be turned on, but I don't think they are. At least worth turning them on in the .VSCode settings.
PlagueHO
reviewed
Sep 26, 2019
There was a problem hiding this comment.
Ok, it's going to take me a little while to look through some of the results here, because it looks like we've got quite a few violations on this one. I'll take a look tonight.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SSvilen)
SSvilen
commented
Sep 27, 2019
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @PlagueHO)
DscResource.AnalyzerRules/DscResource.AnalyzerRules.psm1, line 1077 at r1 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
That is a good point - they should be turned on, but I don't think they are. At least worth turning them on in the .VSCode settings.
I'll do it tomorrow and publish it as part of this PR.
|
Sorry about the delay, but here is the logs of what needs to be fixed (a lot): |
|
List of resources with violations:
|
|
I can do some also - like xexchange or any of the others you have , @PlagueHO? |
|
Just send in PR for any you like :) |
|
I have a bunch of reviews to do this weekend so I probably won’t have anytime until next weekend. @SSvilen you may resolve xWebAdministration if you like. |
SSvilen
commented
Oct 5, 2019
There was a problem hiding this comment.
I just have one question - is this better
Write-FunctionEntry -Parameters @{'MailboxServer' = $MailboxServer; 'DAGName' = $DAGName } -Verbose:$VerbosePreferenceor this
Write-FunctionEntry -Parameters @{
'MailboxServer' = $MailboxServer
'DAGName' = $DAGName
} -Verbose:$VerbosePreferenceDon't know - looks like an edge case to me.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @PlagueHO)
|
We should format the hash table as your later example to follow the style guideline. Not using semicolon. |
|
Regarding the empty hash table format, there is an issue open in PSSA relating to this: PowerShell/PSScriptAnalyzer#1346 |
This was referenced Oct 6, 2019
|
I added these issues so that someone can pick them up (before I have time to resolve them) |
|
Yes, let’s merge this. I will fix any problem in any repo that I maintain. And in others if there is an issue with release or something |
SSvilen
commented
Oct 20, 2019
There was a problem hiding this comment.
Done.
Reviewable status: 3 of 6 files reviewed, all discussions resolved (waiting on @PlagueHO)
PlagueHO
approved these changes
Oct 21, 2019
There was a problem hiding this comment.
Reviewed 3 of 3 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
|
@SSvilen - LOL, I was about to merge this but noticed that the tests are failing because the Hash Tables are in the wrong format 😁 https://ci.appveyor.com/project/PowerShell/dscresource-tests/builds/28245419?fullLog=true#L307 Can you fix these? |
|
@SSvilen - are you still working on this one? |
SSvilen
commented
Oct 26, 2019
There was a problem hiding this comment.
Sorry @PlagueHO - that must have slipped through... I thought that there are still DSC Repos, that have to be fixed.
Reviewable status: 6 of 8 files reviewed, all discussions resolved
PlagueHO
approved these changes
Oct 26, 2019
There was a problem hiding this comment.
No worries at all @SSvilen! I think they're all either done now or we're just going to deal with them once this is through.
Reviewed 2 of 2 files at r5.
Reviewable status:
PlagueHO
approved these changes
Oct 26, 2019
There was a problem hiding this comment.
Reviewable status:
|
Tagging @johlju - this is now through, so if there are any outstanding repos impacted by this then we'll need to prioritize fixing them. But I think they're mostly all done. |
|
This is failing empty hashtables that are written |
|
The formatting is actually done by PSScriptAnalyzer. |
|
Ok, couldn't we have held off on this change until PSScriptAnalyzer was fixed then? |
|
@X-Guardian - that is my bad. It slipped my mind that the PSSA fix hadn't gone out yet (as it had been fixed - just not released). Could we put a temporary fix in that accepts @{ } until the PSSA change is released? |
|
Yes let’s do that, because it can take a while before the new stable PSSA is shipped with VSCode PS extension. PSSA can be installed into the user module folder but all users might not do that, so let’s add an override (temporary workaround) for that. |
|
The How are you wanting this to be formatted? |
|
PR comes soon. The correct way would be: Set-ADObject -Identity $spnAccount.DistinguishedName -Remove @{
ServicePrincipalName = $ServicePrincipalName
} |
|
Everyone happy with this? Personally, when it is a single key/value pair hashtable, the one line format looks better to me. |
|
I think we already had that discussion. But the guidelines are clear about it. PR has been opened. |
|
@X-Guardian Yep, We are happy with what @SSvilen said. Either according to how he showed, or add the hashtable to a variable and use that in the |
|
@PlagueHO I will go through the modules that haven't been resolved in your list. Starting with those that will be released in a day or so. |
|
Thanks @johlju - again, sorry about that - I got my wires crossed and thought that all the other resources had been completed. |
|
@PlagueHO No worries, my mistake! I was suppose to fix those last weekend. Totally forgot. But thanks to @SSvilen most are already fixed. I just sent in a PR to xBitlocker and I'm working on SqlServerDsc which is the only one left. It will not be released this time so there is no hurry, but I'm bored at a hotel today so I working on fixing that right now. 😄 |
|
All modules should now be resolved or are pending a PR that will resolve the module. Now we can start with the other PSSA rules @SSvilen has sent in a PR for. 😄 |
|
Thanks @johlju ! Glad you could be entertained at the hotel 😁 |
|
xDnsServer is failing its meta-tests with several hashtable format errors that were missed for this. I've raised PR dsccommunity/DnsServerDsc#119 to fix these, but the AppVeyor config for the repo is now not correct and is not running... |
|
I take it back, the AppVeyor ran, it's just the licence/cla check that is still waiting. Can someone do a quick review of this if they have time? |
|
@X-Guardian - yep. Tag me in it and I'll get it done. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request (PR) description
Hashtables and Objects should be written in the following format. Each property should be on its own line indented once.
$hashtable = @{
Key1 = 'Value1'
Key2 = 2
Key3 = '3'
}
This Pull Request (PR) fixes the following issues
None
Task list
Entry should say what was changed, and how that affects users (if applicable).
This change is