Conversation
|
Warning Rate limit exceeded@johlju has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughRefactors across multiple PowerShell helper functions: several fixed-size array collectors replaced with System.Collections.ArrayList, SuppressMessage attributes added to multiple public functions, Compare-DscParameterState gained stricter validation and unified CIM handling, Remove-CommonParameter replaced pipeline logic with explicit loops, and Clear-ZeroedEnumPropertyValue adopted a begin/process/end lifecycle. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Compare as Compare-DscParameterState
participant Validator as TypeValidator
participant Converter as ConvertTo-Hashtable
participant Tester as Test-DscParameterState
Caller->>Compare: Invoke(CurrentValues, DesiredValues, Properties, ...)
activate Compare
Compare->>Validator: Validate CurrentValues type
Validator-->>Compare: ok / throw
Compare->>Validator: Validate DesiredValues type
Validator-->>Compare: ok / throw
alt CurrentValues is CimInstance
Compare->>Converter: ConvertTo-Hashtable(CurrentValues)
Converter-->>Compare: Hashtable
end
alt DesiredValues is CimInstance
Compare->>Converter: ConvertTo-Hashtable(DesiredValues)
Converter-->>Compare: Hashtable
end
Compare->>Compare: Build key list (remove common params, apply includes/excludes)
loop For each key
Compare->>Tester: Test-DscParameterState @params
Tester-->>Compare: per-key result
Compare->>Compare: Append to ArrayList
end
Compare-->>Caller: Return ArrayList entries as PSCustomObject[]
deactivate Compare
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
source/Private/Clear-ZeroedEnumPropertyValue.ps1 (2)
30-54: Pipeline behavior regression: function now aggregates multiple pipeline inputs into a single output.With begin/process/end, $result is initialized in begin and returned once in end, merging all pipeline InputObject items. Previous behavior (per typical ValueFromPipeline = $true pattern) likely returned one hashtable per input. Emit per input from process and avoid aggregating across pipeline.
Apply this diff to restore one-output-per-input while keeping the structure:
-begin -{ - $result = @{} -} +begin +{ +} -process -{ +process +{ + $result = @{} @@ -} + return $result +} -end -{ - return $result -} +end +{ +}
2-9: Fix help text typo (“hashable” → “hashtable”).Spelling in SYNOPSIS/DESCRIPTION should match the parameter type.
-Removes any properties from a hashable which have values that are +Removes any properties from a hashtable which have values that are @@ -Removes any properties from a hashable which have values that are +Removes any properties from a hashtable which have values that aresource/Private/Test-DscPropertyState.ps1 (1)
61-138: ArrayList used with += defeats the perf goal; use .Add() instead.
$arr += xcreates a new Object[] each time, negating the ArrayList optimization. Use.Add()and discard the return value.- $propertyState = [System.Collections.ArrayList]::new() + $propertyState = [System.Collections.ArrayList]::new() @@ - # Recursively call Test-DscPropertyState with the CimInstance to evaluate. - $propertyState += Test-DscPropertyState -Values @{ - CurrentValue = $currentCimInstance - DesiredValue = $desiredCimInstance - } + # Recursively call Test-DscPropertyState with the CimInstance to evaluate. + [void] $propertyState.Add( + (Test-DscPropertyState -Values @{ + CurrentValue = $currentCimInstance + DesiredValue = $desiredCimInstance + }) + ) @@ - $propertyState = [System.Collections.ArrayList]::new() + $propertyState = [System.Collections.ArrayList]::new() @@ - $propertyState += Test-DscPropertyState -Values @{ - CurrentValue = $Values.CurrentValue.($desiredCimInstanceProperty.Name) - DesiredValue = $desiredCimInstanceProperty.Value - } + [void] $propertyState.Add( + (Test-DscPropertyState -Values @{ + CurrentValue = $Values.CurrentValue.($desiredCimInstanceProperty.Name) + DesiredValue = $desiredCimInstanceProperty.Value + }) + ) @@ - $propertyState += $false + [void] $propertyState.Add($false)Also applies to: 141-175
source/Public/Find-Certificate.ps1 (1)
134-174: Use ArrayList.Add/AddRange instead of += to realize the perf win.Using += on ArrayList coerces to a new array and defeats the intended optimization. Switch to .Add for single items.
Apply:
- $certFilters = [System.Collections.ArrayList]::new() + $certFilters = [System.Collections.ArrayList]::new() @@ - $certFilters += @('($_.Thumbprint -eq $Thumbprint)') + [void] $certFilters.Add('($_.Thumbprint -eq $Thumbprint)') @@ - $certFilters += @('($_.FriendlyName -eq $FriendlyName)') + [void] $certFilters.Add('($_.FriendlyName -eq $FriendlyName)') @@ - $certFilters += @('($_.Subject -eq $Subject)') + [void] $certFilters.Add('($_.Subject -eq $Subject)') @@ - $certFilters += @('($_.Issuer -eq $Issuer)') + [void] $certFilters.Add('($_.Issuer -eq $Issuer)') @@ - $certFilters += @('(((Get-Date) -le $_.NotAfter) -and ((Get-Date) -ge $_.NotBefore))') + [void] $certFilters.Add('(((Get-Date) -le $_.NotAfter) -and ((Get-Date) -ge $_.NotBefore))') @@ - $certFilters += @('(@(Compare-Object -ReferenceObject $_.DNSNameList.Unicode -DifferenceObject $DNSName | Where-Object -Property SideIndicator -eq "=>").Count -eq 0)') + [void] $certFilters.Add('(@(Compare-Object -ReferenceObject $_.DNSNameList.Unicode -DifferenceObject $DNSName | Where-Object -Property SideIndicator -eq "=>").Count -eq 0)') @@ - $certFilters += @('(@(Compare-Object -ReferenceObject ($_.Extensions.KeyUsages -split ", ") -DifferenceObject $KeyUsage | Where-Object -Property SideIndicator -eq "=>").Count -eq 0)') + [void] $certFilters.Add('(@(Compare-Object -ReferenceObject ($_.Extensions.KeyUsages -split ", ") -DifferenceObject $KeyUsage | Where-Object -Property SideIndicator -eq "=>").Count -eq 0)') @@ - $certFilters += @('(@(Compare-Object -ReferenceObject ($_.EnhancedKeyUsageList.FriendlyName) -DifferenceObject $EnhancedKeyUsage | Where-Object -Property SideIndicator -eq "=>").Count -eq 0)') + [void] $certFilters.Add('(@(Compare-Object -ReferenceObject ($_.EnhancedKeyUsageList.FriendlyName) -DifferenceObject $EnhancedKeyUsage | Where-Object -Property SideIndicator -eq "=>").Count -eq 0)')source/Public/Compare-ResourcePropertyState.ps1 (1)
206-241: Replace ArrayList += with .Add to avoid copies.Using += on an ArrayList creates intermediate arrays; use .Add for O(1) append.
Apply:
- $compareTargetResourceStateReturnValue = [System.Collections.ArrayList]::new() + $compareTargetResourceStateReturnValue = [System.Collections.ArrayList]::new() @@ - $compareTargetResourceStateReturnValue += $parameterState + [void] $compareTargetResourceStateReturnValue.Add($parameterState)source/Public/Get-DscProperty.ps1 (1)
140-167: Use ArrayList.AddRange instead of += for accumulated filters.Avoids array reallocations and preserves the ArrayList type.
Apply:
- $propertiesOfAttribute = [System.Collections.ArrayList]::new() - - $propertiesOfAttribute += $property | Where-Object -FilterScript { + $propertiesOfAttribute = [System.Collections.ArrayList]::new() + [void] $propertiesOfAttribute.AddRange( + ($property | Where-Object -FilterScript { $InputObject.GetType().GetMember($_).CustomAttributes.Where( { <# To simplify the code, ignoring that this will compare MemberNAme against type 'Optional' which does not exist. #> $_.NamedArguments.MemberName -in $Attribute } ).NamedArguments.TypedValue.Value -eq $true - } + }) + ) @@ - $propertiesOfAttribute += $property | Where-Object -FilterScript { + [void] $propertiesOfAttribute.AddRange( + ($property | Where-Object -FilterScript { $InputObject.GetType().GetMember($_).CustomAttributes.Where( { $_.NamedArguments.MemberName -notin @('Key', 'Mandatory', 'NotConfigurable') } ) - } + }) + )source/Public/Compare-DscParameterState.ps1 (2)
187-201: Fix ArrayList usage and filtering to actually realize the perf gain.
$returnValueis an ArrayList, but using+=converts it into a fixedobject[], defeating the purpose and potentially hurting perf. Also, calling.Where()on an ArrayList isn’t reliably supported on Windows PowerShell 5.1. Use.Add()and filter viaWhere-Object(or cast to[array]first) to keep compatibility and performance.Apply:
@@ - $returnValue = [System.Collections.ArrayList]::new() + $returnValue = [System.Collections.ArrayList]::new() @@ - $returnValue += $InDesiredStateTable + $null = $returnValue.Add($InDesiredStateTable) @@ - if (-not $IncludeInDesiredState) - { - [array]$returnValue = $returnValue.Where({ $_.InDesiredState -eq $false }) - } + if (-not $IncludeInDesiredState) + { + [array]$returnValue = @($returnValue | Where-Object { $_.InDesiredState -eq $false }) + }Also applies to: 256-263, 626-631
258-271: Keep output contract: ExpectedType/ActualType must be strings (per .OUTPUTS doc).Currently you emit a Type object or a hashtable with
Name='Unknown'. The docs specify strings. This can break consumers.@@ - $InDesiredStateTable['ExpectedValue'] = $desiredValue - $InDesiredStateTable['ActualValue'] = $currentValue + $InDesiredStateTable['ExpectedValue'] = $desiredValue + $InDesiredStateTable['ActualValue'] = $currentValue @@ - $InDesiredStateTable['ExpectedType'] = $desiredType + $InDesiredStateTable['ExpectedType'] = if ($desiredType -is [Type]) { $desiredType.FullName } else { $desiredType.Name } @@ - $InDesiredStateTable['ActualType'] = $currentType + $InDesiredStateTable['ActualType'] = if ($currentType -is [Type]) { $currentType.FullName } else { $currentType.Name }I can add/update unit tests to assert these fields are strings across scalar, array, hashtable, PSCredential, and ScriptBlock scenarios.
Also applies to: 298-299, 311-312
🧹 Nitpick comments (4)
source/Public/Compare-DscParameterState.ps1 (4)
247-251: Avoid.Where()on key collections for PS 5.1 compat.
$desiredValuesClean.Keysmay not support accelerated.Where()on PS 5.1. PreferWhere-Object(or cast to[array]first).- $keyList = $keyList.Where({ $_ -notin $ExcludeProperties }) + $keyList = @($keyList | Where-Object { $_ -notin $ExcludeProperties })Would you like me to run a quick cross-version check matrix (PS 5.1 vs 7.x) script?
204-210: Prefer type checks over string name comparisons for robustness.String-based type checks are brittle. Use
-isagainst concrete types.- $types = @( - 'System.Management.Automation.PSBoundParametersDictionary' - 'System.Collections.Hashtable' - 'Microsoft.Management.Infrastructure.CimInstance' - 'System.Collections.Specialized.OrderedDictionary' - ) - - if ($DesiredValues.GetType().FullName -notin $types) + if (-not ( + $DesiredValues -is [System.Collections.Hashtable] -or + $DesiredValues -is [System.Collections.Specialized.OrderedDictionary] -or + $DesiredValues -is [System.Management.Automation.PSBoundParametersDictionary] -or + $DesiredValues -is [Microsoft.Management.Infrastructure.CimInstance] + )) { @@ - if ($CurrentValues.GetType().FullName -notin $types) + if (-not ( + $CurrentValues -is [System.Collections.Hashtable] -or + $CurrentValues -is [System.Collections.Specialized.OrderedDictionary] -or + $CurrentValues -is [System.Management.Automation.PSBoundParametersDictionary] -or + $CurrentValues -is [Microsoft.Management.Infrastructure.CimInstance] + )) {Also applies to: 211-224
315-342: Credential comparison path looks correct; consider minimizing PII in logs.Verbose messages include usernames. Consider masking or only logging presence of match to reduce PII exposure.
567-591: Name the conversion flag clearly.Minor clarity: rename
$wasCurrentValueto$wasCurrentValueConvertedto mirror the array branch and intent.- $wasCurrentValue = $false + $wasCurrentValueConverted = $false @@ - $wasCurrentValue = $true + $wasCurrentValueConverted = $true @@ - if ($desiredValue -is [scriptblock]) + if ($desiredValue -is [scriptblock]) { - $desiredValue = if ($currentValue -is [string] -and -not $wasCurrentValue) + $desiredValue = if ($currentValue -is [string] -and -not $wasCurrentValueConverted)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
source/Private/Clear-ZeroedEnumPropertyValue.ps1(2 hunks)source/Private/Test-DscPropertyState.ps1(2 hunks)source/Public/Assert-BoundParameter.ps1(1 hunks)source/Public/Compare-DscParameterState.ps1(17 hunks)source/Public/Compare-ResourcePropertyState.ps1(2 hunks)source/Public/ConvertFrom-DscResourceInstance.ps1(1 hunks)source/Public/Find-Certificate.ps1(1 hunks)source/Public/Get-DscProperty.ps1(3 hunks)source/Public/Get-UserName.ps1(1 hunks)source/Public/Remove-CommonParameter.ps1(1 hunks)source/Public/Test-DscProperty.ps1(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**
⚙️ CodeRabbit configuration file
**: # DSC Community GuidelinesTerminology
- Command: Public command
- Function: Private function
- Resource: DSC class-based resource
Build & Test Workflow
- Run project scripts in PowerShell from repository root
- Build after source changes:
.\build.ps1 -Tasks build- Test workflow: Build →
Invoke-Pester -Path @('<test paths>') -Output Detailed- New session required after class changes
File Organization
- Public commands:
source/Public/{CommandName}.ps1- Private functions:
source/Private/{FunctionName}.ps1- Unit tests:
tests/Unit/{Classes|Public|Private}/{Name}.Tests.ps1- Integration tests:
tests/Integration/Commands/{CommandName}.Integration.Tests.ps1Requirements
- Follow guidelines over existing code patterns
- Always update CHANGELOG.md Unreleased section
- Localize all strings using string keys; remove any orphaned string keys
- Check DscResource.Common before creating private functions
- Separate reusable logic into private functions
- Add unit tests for all commands/functions/resources
- Add integration tests for all public commands and resources
Files:
source/Public/Compare-ResourcePropertyState.ps1source/Public/ConvertFrom-DscResourceInstance.ps1source/Public/Get-DscProperty.ps1source/Private/Test-DscPropertyState.ps1source/Public/Get-UserName.ps1source/Public/Remove-CommonParameter.ps1source/Public/Find-Certificate.ps1source/Public/Test-DscProperty.ps1source/Private/Clear-ZeroedEnumPropertyValue.ps1source/Public/Compare-DscParameterState.ps1source/Public/Assert-BoundParameter.ps1
**/*.ps?(m|d)1
⚙️ CodeRabbit configuration file
**/*.ps?(m|d)1: # PowerShell GuidelinesNaming
- Use descriptive names (3+ characters, no abbreviations)
- Functions: PascalCase with Verb-Noun format using approved verbs
- Parameters: PascalCase
- Variables: camelCase
- Keywords: lower-case
- Classes: PascalCase
- Include scope for script/global/environment variables:
$script:,$global:,$env:File naming
- Class files:
###.ClassName.ps1format (e.g.001.SqlReason.ps1,004.StartupParameters.ps1)Formatting
Indentation & Spacing
- Use 4 spaces (no tabs)
- One space around operators:
$a = 1 + 2- One space between type and variable:
[String] $name- One space between keyword and parenthesis:
if ($condition)- No spaces on empty lines
- Try to limit lines to 120 characters
Braces
- Newline before opening brace (except variable assignments)
- One newline after opening brace
- Two newlines after closing brace (one if followed by another brace or continuation)
Quotes
- Use single quotes unless variable expansion is needed:
'text'vs"text $variable"Arrays
- Single line:
@('one', 'two', 'three')- Multi-line: each element on separate line with proper indentation
- Do not use the unary comma operator (
,) in return statements to force
an arrayHashtables
- Empty:
@{}- Multi-line: each property on separate line with proper indentation
- Properties: Use PascalCase
Comments
- Single line:
# Comment(capitalized, on own line)- Multi-line:
<# Comment #>format (opening and closing brackets on own line), and indent text- No commented-out code
Comment-based help
- Always add comment-based help to all functions and scripts
- Comment-based help: SYNOPSIS, DESCRIPTION (40+ chars), PARAMETER, EXAMPLE sections before function/class
- Comment-based help indentation: keywords 4 spaces, text 8 spaces
- Include examples for all parameter sets and combinations
- INPUTS: List each pipeline‑accepted type (one per line) with a 1‑line description.
- ...
Files:
source/Public/Compare-ResourcePropertyState.ps1source/Public/ConvertFrom-DscResourceInstance.ps1source/Public/Get-DscProperty.ps1source/Private/Test-DscPropertyState.ps1source/Public/Get-UserName.ps1source/Public/Remove-CommonParameter.ps1source/Public/Find-Certificate.ps1source/Public/Test-DscProperty.ps1source/Private/Clear-ZeroedEnumPropertyValue.ps1source/Public/Compare-DscParameterState.ps1source/Public/Assert-BoundParameter.ps1
source/**/*.ps1
⚙️ CodeRabbit configuration file
source/**/*.ps1: # Localization GuidelinesRequirements
- Localize all Write-Debug, Write-Verbose, Write-Error, Write-Warning and $PSCmdlet.ThrowTerminatingError() messages
- Use localized string keys, not hardcoded strings
- Assume
$script:localizedDatais availableString Files
- Commands/functions:
source/en-US/SqlServerDsc.strings.psd1- Class resources:
source/en-US/{ResourceClassName}.strings.psd1Key Naming Patterns
- Format:
Verb_FunctionName_Action(underscore separators), e.g.Get_SqlDscDatabase_ConnectingToDatabaseString Format
ConvertFrom-StringData @' KeyName = Message with {0} placeholder. (PREFIX0001) '@String IDs
- Format:
(PREFIX####)- PREFIX: First letter of each word in class or function name (SqlSetup → SS, Get-SqlDscDatabase → GSDD)
- Number: Sequential from 0001
Usage
Write-Verbose -Message ($script:localizedData.KeyName -f $value1)
Files:
source/Public/Compare-ResourcePropertyState.ps1source/Public/ConvertFrom-DscResourceInstance.ps1source/Public/Get-DscProperty.ps1source/Private/Test-DscPropertyState.ps1source/Public/Get-UserName.ps1source/Public/Remove-CommonParameter.ps1source/Public/Find-Certificate.ps1source/Public/Test-DscProperty.ps1source/Private/Clear-ZeroedEnumPropertyValue.ps1source/Public/Compare-DscParameterState.ps1source/Public/Assert-BoundParameter.ps1
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: dsccommunity.DscResource.Common (Build Package Module)
🔇 Additional comments (12)
source/Public/Get-UserName.ps1 (1)
3-9: Typo fixes LGTM. No functional changes; docs now read correctly.source/Public/Assert-BoundParameter.ps1 (1)
156-156: Suppression attribute addition LGTM.Appropriate placement and clear justification; no API/behavioral change.
source/Public/ConvertFrom-DscResourceInstance.ps1 (1)
43-43: LGTM: appropriate suppression placement.Attribute is correctly scoped before CmdletBinding(); no runtime impact.
source/Public/Test-DscProperty.ps1 (2)
11-11: Doc fix looks good.Corrected wording improves clarity.
65-65: LGTM: appropriate suppression placement.Consistent with other public functions; no behavior change.
source/Public/Compare-ResourcePropertyState.ps1 (1)
134-134: LGTM: suppression added without functional change.source/Public/Get-DscProperty.ps1 (2)
76-76: LGTM: suppression adds no runtime impact.
177-181: LGTM: HasValue path simplification.Direct call to Test-DscPropertyIsAssigned reduces temp vars and reads cleanly.
source/Public/Compare-DscParameterState.ps1 (4)
490-503: LGTM: Nested hashtable/ordered recursion and bound-param sanitization.Recursing via
Test-DscParameterStatefor complex array elements and strippingIncludeInDesiredState/IncludeValue/Properties/ReverseCheckis correct and prevents misbinding.Also applies to: 510-512
529-541: LGTM: Hashtable recursion with empty-desired guard.Good defensive check for empty desired vs non-empty current before recursing.
Also applies to: 546-557
636-647: LGTM: Conditional verbose messaging under IncludeInDesiredState.Clean, localized, and uses ForEach accelerated method correctly.
657-657: LGTM: Canonical [PSCustomObject] casing.Consistent with style and avoids type resolution edge cases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
CHANGELOG.md (7)
17-18: Clarify wording and note potential impactTighten phrasing and format type/commands; also flag potential behavioral change for consumers relying on array semantics.
Please confirm whether the return/container change to ArrayList is non-breaking for downstream consumers; if not, consider labeling as a breaking change in the note.
- - Use ArrayList and not fixed size array. - - Remove some ForEach-Object usage. + - Use `System.Collections.ArrayList` instead of fixed-size arrays (may impact callers relying on array semantics). + - Replace some use of `ForEach-Object` with explicit `foreach` loops to reduce pipeline overhead.
26-26: Wording/style: specify type and hyphenateConsistent with style used elsewhere.
- - Use ArrayList and not fixed size array. + - Use `System.Collections.ArrayList` instead of fixed-size arrays.
33-34: Wording/style: specify type and hyphenateMirror prior phrasing.
- - Use ArrayList and not fixed size array. + - Use `System.Collections.ArrayList` instead of fixed-size arrays.
35-36: Expand acronym on first useSpell out PSSA once in this section for clarity.
- - Fix PSSA warning. + - Fix PowerShell Script Analyzer (PSSA) warning.
37-38: Align phrasing with prior bulletOptional consistency with expanded acronym.
- - Fix PSSA warning. + - Fix PowerShell Script Analyzer (PSSA) warning.
39-41: Wording/style and acronym expansionConsistent terminology and readability.
- - Use ArrayList and not fixed size array. - - Fix PSSA warning. + - Use `System.Collections.ArrayList` instead of fixed-size arrays. + - Fix PowerShell Script Analyzer (PSSA) warning.
42-43: Prefer active voice and specify replacementClarify what replaced pipeline usage and why.
- - Remove use of `Where-Object` and `ForEach-Object`. + - Replace `Where-Object` and `ForEach-Object` with explicit `foreach` loops to reduce pipeline overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**
⚙️ CodeRabbit configuration file
**: # DSC Community GuidelinesTerminology
- Command: Public command
- Function: Private function
- Resource: DSC class-based resource
Build & Test Workflow
- Run project scripts in PowerShell from repository root
- Build after source changes:
.\build.ps1 -Tasks build- Test workflow: Build →
Invoke-Pester -Path @('<test paths>') -Output Detailed- New session required after class changes
File Organization
- Public commands:
source/Public/{CommandName}.ps1- Private functions:
source/Private/{FunctionName}.ps1- Unit tests:
tests/Unit/{Classes|Public|Private}/{Name}.Tests.ps1- Integration tests:
tests/Integration/Commands/{CommandName}.Integration.Tests.ps1Requirements
- Follow guidelines over existing code patterns
- Always update CHANGELOG.md Unreleased section
- Localize all strings using string keys; remove any orphaned string keys
- Check DscResource.Common before creating private functions
- Separate reusable logic into private functions
- Add unit tests for all commands/functions/resources
- Add integration tests for all public commands and resources
Files:
CHANGELOG.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: # Markdown Style Guidelines
- Wrap lines at word boundaries when over 80 characters (except tables/code blocks)
- Use 2 spaces for indentation
- Use '1.' for all items in ordered lists (1/1/1 numbering style)
- Disable
MD013rule by adding a comment for tables/code blocks exceeding 80 characters- Empty lines required before/after code blocks and headings (except before line 1)
- Escape backslashes in file paths only (not in code blocks)
- Code blocks must specify language identifiers
Text Formatting
- Parameters: bold
- Values/literals:
inline code- Resource/module/product names: italic
- Commands/files/paths:
inline code
Files:
CHANGELOG.md
CHANGELOG.md
⚙️ CodeRabbit configuration file
CHANGELOG.md: # Changelog Guidelines
- Always update the Unreleased section in CHANGELOG.md
- Use Keep a Changelog format
- Describe notable changes briefly, ≤2 items per change type
- Reference issues using format #<issue_number>
- No empty lines between list items in same section
- Do not add item if there are already an existing item for the same change
Files:
CHANGELOG.md
🪛 LanguageTool
CHANGELOG.md
[grammar] ~17-~17: There might be a mistake here.
Context: ... Use ArrayList and not fixed size array. - Remove some ForEach-Object usage. - `Tes...
(QB_NEW_EN)
[style] ~25-~25: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Write-Debug. - Find-Certificate - Changed certificate filter diagnostic message f...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~39-~39: There might be a mistake here.
Context: ... Use ArrayList and not fixed size array. - Fix PSSA warning. - `Remove-CommonParame...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: dsccommunity.DscResource.Common (Build Package Module)
|
Waiting for #171 to merge with GitVersion fix. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
source/Public/Remove-CommonParameter.ps1 (1)
33-39: Good fix: case‑insensitive O(1) lookups with HashSet resolve prior nested array bug.This addresses the earlier issue where
-innever matched due to a nested array. The OrdinalIgnoreCase comparer is appropriate for parameter names.CHANGELOG.md (1)
30-31: Resolved: “begin” block typo fixed.Previous note about “before” → “begin” is addressed here.
🧹 Nitpick comments (4)
source/Public/Remove-CommonParameter.ps1 (1)
41-46: Cast keys to string to avoid type-coercion surprises and tiny perf hit.Safer when non-string keys slip in; keeps the HashSet generic type contract explicit.
- if ($commonParameters.Contains($key)) + if ($commonParameters.Contains([string]$key))Optionally cache the set at script scope to avoid reallocation on each call (only if this runs hot):
# At top of file (outside the function) if (-not $script:CommonParameterNameSet) { $script:CommonParameterNameSet = [System.Collections.Generic.HashSet[System.String]]::new( [System.String[]]( [System.Management.Automation.PSCmdlet]::CommonParameters + [System.Management.Automation.PSCmdlet]::OptionalCommonParameters ), [System.StringComparer]::OrdinalIgnoreCase ) }Then inside the function, reuse
$script:CommonParameterNameSet.CHANGELOG.md (3)
17-18: Polish wording: hyphenate and use parallel phrasing.Improve readability and style.
- - Use ArrayList and not fixed size array. - - Remove some ForEach-Object usage. + - Use `ArrayList` instead of fixed-size arrays. + - Removed some occurrences of `ForEach-Object`.
26-26: Consistency: same phrasing for similar bullets.- - Use ArrayList and not fixed size array. + - Use `ArrayList` instead of fixed-size arrays.
33-42: Tighten wording across related bullets for uniform style.- - Use ArrayList and not fixed size array. + - Use `ArrayList` instead of fixed-size arrays. @@ - - Fix PSSA warning. + - Fixed PSSA warning. @@ - - Use ArrayList and not fixed size array. + - Use `ArrayList` instead of fixed-size arrays. @@ - - Remove use of `Where-Object` and `ForEach-Object`. + - Replaced `Where-Object`/`ForEach-Object` with explicit loops for performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)source/Private/Clear-ZeroedEnumPropertyValue.ps1(2 hunks)source/Public/Remove-CommonParameter.ps1(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- source/Private/Clear-ZeroedEnumPropertyValue.ps1
🧰 Additional context used
📓 Path-based instructions (5)
**
⚙️ CodeRabbit configuration file
**: # DSC Community GuidelinesTerminology
- Command: Public command
- Function: Private function
- Resource: DSC class-based resource
Build & Test Workflow
- Run project scripts in PowerShell from repository root
- Build after source changes:
.\build.ps1 -Tasks build- Test workflow: Build →
Invoke-Pester -Path @('<test paths>') -Output Detailed- New session required after class changes
File Organization
- Public commands:
source/Public/{CommandName}.ps1- Private functions:
source/Private/{FunctionName}.ps1- Unit tests:
tests/Unit/{Classes|Public|Private}/{Name}.Tests.ps1- Integration tests:
tests/Integration/Commands/{CommandName}.Integration.Tests.ps1Requirements
- Follow guidelines over existing code patterns
- Always update CHANGELOG.md Unreleased section
- Localize all strings using string keys; remove any orphaned string keys
- Check DscResource.Common before creating private functions
- Separate reusable logic into private functions
- Add unit tests for all commands/functions/resources
- Add integration tests for all public commands and resources
Files:
source/Public/Remove-CommonParameter.ps1CHANGELOG.md
**/*.ps?(m|d)1
⚙️ CodeRabbit configuration file
**/*.ps?(m|d)1: # PowerShell GuidelinesNaming
- Use descriptive names (3+ characters, no abbreviations)
- Functions: PascalCase with Verb-Noun format using approved verbs
- Parameters: PascalCase
- Variables: camelCase
- Keywords: lower-case
- Classes: PascalCase
- Include scope for script/global/environment variables:
$script:,$global:,$env:File naming
- Class files:
###.ClassName.ps1format (e.g.001.SqlReason.ps1,004.StartupParameters.ps1)Formatting
Indentation & Spacing
- Use 4 spaces (no tabs)
- One space around operators:
$a = 1 + 2- One space between type and variable:
[String] $name- One space between keyword and parenthesis:
if ($condition)- No spaces on empty lines
- Try to limit lines to 120 characters
Braces
- Newline before opening brace (except variable assignments)
- One newline after opening brace
- Two newlines after closing brace (one if followed by another brace or continuation)
Quotes
- Use single quotes unless variable expansion is needed:
'text'vs"text $variable"Arrays
- Single line:
@('one', 'two', 'three')- Multi-line: each element on separate line with proper indentation
- Do not use the unary comma operator (
,) in return statements to force
an arrayHashtables
- Empty:
@{}- Multi-line: each property on separate line with proper indentation
- Properties: Use PascalCase
Comments
- Single line:
# Comment(capitalized, on own line)- Multi-line:
<# Comment #>format (opening and closing brackets on own line), and indent text- No commented-out code
Comment-based help
- Always add comment-based help to all functions and scripts
- Comment-based help: SYNOPSIS, DESCRIPTION (40+ chars), PARAMETER, EXAMPLE sections before function/class
- Comment-based help indentation: keywords 4 spaces, text 8 spaces
- Include examples for all parameter sets and combinations
- INPUTS: List each pipeline‑accepted type (one per line) with a 1‑line description.
- ...
Files:
source/Public/Remove-CommonParameter.ps1
source/**/*.ps1
⚙️ CodeRabbit configuration file
source/**/*.ps1: # Localization GuidelinesRequirements
- Localize all Write-Debug, Write-Verbose, Write-Error, Write-Warning and $PSCmdlet.ThrowTerminatingError() messages
- Use localized string keys, not hardcoded strings
- Assume
$script:localizedDatais availableString Files
- Commands/functions:
source/en-US/SqlServerDsc.strings.psd1- Class resources:
source/en-US/{ResourceClassName}.strings.psd1Key Naming Patterns
- Format:
Verb_FunctionName_Action(underscore separators), e.g.Get_SqlDscDatabase_ConnectingToDatabaseString Format
ConvertFrom-StringData @' KeyName = Message with {0} placeholder. (PREFIX0001) '@String IDs
- Format:
(PREFIX####)- PREFIX: First letter of each word in class or function name (SqlSetup → SS, Get-SqlDscDatabase → GSDD)
- Number: Sequential from 0001
Usage
Write-Verbose -Message ($script:localizedData.KeyName -f $value1)
Files:
source/Public/Remove-CommonParameter.ps1
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: # Markdown Style Guidelines
- Wrap lines at word boundaries when over 80 characters (except tables/code blocks)
- Use 2 spaces for indentation
- Use '1.' for all items in ordered lists (1/1/1 numbering style)
- Disable
MD013rule by adding a comment for tables/code blocks exceeding 80 characters- Empty lines required before/after code blocks and headings (except before line 1)
- Escape backslashes in file paths only (not in code blocks)
- Code blocks must specify language identifiers
Text Formatting
- Parameters: bold
- Values/literals:
inline code- Resource/module/product names: italic
- Commands/files/paths:
inline code
Files:
CHANGELOG.md
CHANGELOG.md
⚙️ CodeRabbit configuration file
CHANGELOG.md: # Changelog Guidelines
- Always update the Unreleased section in CHANGELOG.md
- Use Keep a Changelog format
- Describe notable changes briefly, ≤2 items per change type
- Reference issues using format #<issue_number>
- No empty lines between list items in same section
- Do not add item if there are already an existing item for the same change
Files:
CHANGELOG.md
🪛 LanguageTool
CHANGELOG.md
[grammar] ~17-~17: There might be a mistake here.
Context: ... Use ArrayList and not fixed size array. - Remove some ForEach-Object usage. - `Tes...
(QB_NEW_EN)
[style] ~25-~25: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Write-Debug. - Find-Certificate - Changed certificate filter diagnostic message f...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~39-~39: There might be a mistake here.
Context: ... Use ArrayList and not fixed size array. - Fix PSSA warning. - `Remove-CommonParame...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: dsccommunity.DscResource.Common (Build Package Module)
|
Hmm, should not be the same issue 🤔 Have you tried adding DscResource.Common/azure-pipelines.yml Line 30 in 2e1a484 |
|
Ha, it did fix the issue. Cool! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
CHANGELOG.md (7)
17-17: Grammar + clarity: “ArrayList” phrasingPrefer “instead of fixed-size arrays” and format the .NET type.
- - Use ArrayList and not fixed size array. + - Use `[System.Collections.ArrayList]` instead of fixed-size arrays.
28-28: Grammar + clarity: “ArrayList” phrasingSame improvement as above for consistency.
- - Use ArrayList and not fixed size array. + - Use `[System.Collections.ArrayList]` instead of fixed-size arrays.
33-33: Wording: use “and” and code-format block namesMinor readability tweak.
- - Add begin, end blocks. + - Add `begin` and `end` blocks.
35-35: Grammar + clarity: “ArrayList” phrasingAlign with earlier bullets.
- - Use ArrayList and not fixed size array. + - Use `[System.Collections.ArrayList]` instead of fixed-size arrays.
37-40: Optional: include PSSA rule IDs for traceabilityConsider noting the specific PSSA rule(s) suppressed/fixed to aid future audits (e.g., “UseSyntacticallyCorrectExamples”), if applicable.
41-42: Grammar + clarity: “ArrayList” phrasingSame consistency/formatting tweak here.
- - Use ArrayList and not fixed size array. + - Use `[System.Collections.ArrayList]` instead of fixed-size arrays.
43-43: Optional: include PSSA rule IDs for traceabilityAs above, consider citing the exact rule(s) addressed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
CHANGELOG.md(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**
⚙️ CodeRabbit configuration file
**: # DSC Community GuidelinesTerminology
- Command: Public command
- Function: Private function
- Resource: DSC class-based resource
Build & Test Workflow
- Run project scripts in PowerShell from repository root
- Build after source changes:
.\build.ps1 -Tasks build- Test workflow: Build →
Invoke-Pester -Path @('<test paths>') -Output Detailed- New session required after class changes
File Organization
- Public commands:
source/Public/{CommandName}.ps1- Private functions:
source/Private/{FunctionName}.ps1- Unit tests:
tests/Unit/{Classes|Public|Private}/{Name}.Tests.ps1- Integration tests:
tests/Integration/Commands/{CommandName}.Integration.Tests.ps1Requirements
- Follow guidelines over existing code patterns
- Always update CHANGELOG.md Unreleased section
- Localize all strings using string keys; remove any orphaned string keys
- Check DscResource.Common before creating private functions
- Separate reusable logic into private functions
- Add unit tests for all commands/functions/resources
- Add integration tests for all public commands and resources
Files:
CHANGELOG.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: # Markdown Style Guidelines
- Wrap lines at word boundaries when over 80 characters (except tables/code blocks)
- Use 2 spaces for indentation
- Use '1.' for all items in ordered lists (1/1/1 numbering style)
- Disable
MD013rule by adding a comment for tables/code blocks exceeding 80 characters- Empty lines required before/after code blocks and headings (except before line 1)
- Escape backslashes in file paths only (not in code blocks)
- Code blocks must specify language identifiers
Text Formatting
- Parameters: bold
- Values/literals:
inline code- Resource/module/product names: italic
- Commands/files/paths:
inline code
Files:
CHANGELOG.md
CHANGELOG.md
⚙️ CodeRabbit configuration file
CHANGELOG.md: # Changelog Guidelines
- Always update the Unreleased section in CHANGELOG.md
- Use Keep a Changelog format
- Describe notable changes briefly, ≤2 items per change type
- Reference issues using format #<issue_number>
- No empty lines between list items in same section
- Do not add item if there are already an existing item for the same change
Files:
CHANGELOG.md
🪛 LanguageTool
CHANGELOG.md
[grammar] ~17-~17: There might be a mistake here.
Context: ... Use ArrayList and not fixed size array. - Remove some ForEach-Object usage. - `Tes...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ... Use ArrayList and not fixed size array. - Fix PSSA warning. - `Remove-CommonParame...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: dsccommunity.DscResource.Common (Build Package Module)
🔇 Additional comments (1)
CHANGELOG.md (1)
44-45: Good: calls out pipeline operator removal for perfThis directly ties to the PR’s performance objective and is clearly worded.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju reviewed 8 of 11 files at r1, 1 of 3 files at r3, 1 of 1 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-hughes)
2 participants
Pull Request (PR) description
Remove some of the slow code in functions used by
DscResource.Base.This Pull Request (PR) fixes the following issues
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
DSC Community Testing Guidelines.
This change is