ResourceBase: Performance optimizations - #47
Merged
Conversation
WalkthroughRefactors internal iterations to explicit foreach loops and replaces array accumulators with generic List<>; adds a local key cache and stable unique sorting in ResourceBase; fixes a variable reference in Resolve-Reason; updates a unit test to set PropertiesNotInDesiredState; allows prerelease resolution for two RequiredModules; adds a changelog entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor TestRunner
participant ResourceBase
participant ConvertFromCompare as ConvertFrom-CompareResult
participant ConvertFromReason as ConvertFrom-Reason
participant ResolveReason as Resolve-Reason
TestRunner->>ResourceBase: Invoke Get()/Set()
ResourceBase->>ResourceBase: cache current-state keys (Get)
ResourceBase->>ConvertFromCompare: build property->expected map (foreach)
ConvertFromCompare-->>ResourceBase: hashtable(Property->ExpectedValue)
ResourceBase->>ConvertFromReason: collect reasons (List.Add)
ConvertFromReason-->>ResourceBase: [Reason[]]
ResourceBase->>ResolveReason: normalize reasons (List.Add, enum check fix)
ResolveReason-->>ResourceBase: [Reason[]]
ResourceBase-->>TestRunner: return results/status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
==================================
Coverage 99% 99%
==================================
Files 7 7
Lines 133 133
==================================
Hits 132 132
Misses 1 1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
source/Private/Resolve-Reason.ps1 (4)
63-71: Bug: wrong variable used for enum check breaks ActualValue handlingThe enum check uses $property.ActualValue instead of $currentProperty.ActualValue. This prevents enum-to-string conversion and can yield inconsistent JSON output.
Apply this diff:
- if ($property.ActualValue -is [System.Enum]) + if ($currentProperty.ActualValue -is [System.Enum])
116-119: Preserve output contract ([Reason[]]) — convert List to array before returningOutputType and comment-based help declare [Reason[]], but returning the List directly can change caller semantics and break expectations. Return an array for consistency and to avoid a subtle breaking change.
Apply this diff:
- return $reasons + return $reasons.ToArray()
3-3: Typo: “a array” → “an array”Small grammar fix in SYNOPSIS.
Apply this diff:
- Returns a array of the type `[Reason]`. + Returns an array of the type `[Reason]`.
22-24: Add missing test assertions and align return typeThe function should return an actual
[Reason[]](not anObject[]) by using$reasons.ToArray(). In addition, enhance the existing tests intests/Unit/Private/Resolve-Reason.Tests.ps1to lock down the output contract:• Update the cmdlet to return
$reasons.ToArray()so its output type matches[Reason[]].
• In tests/Unit/Private/Resolve-Reason.Tests.ps1, add Pester assertions to verify:
- Return type is
[Reason[]]in both empty- and populated-property cases.ExpectedValueandActualValueenum members are converted to their string names.- Any file paths are unescaped (e.g.
\\server\\share→\server\share).- Null‐input behavior differs as intended between Windows PowerShell Desktop and PowerShell Core.
Let me know if you’d like help with the Pester syntax or example test snippets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (6)
CHANGELOG.md(1 hunks)source/Classes/010.ResourceBase.ps1(1 hunks)source/Private/ConvertFrom-CompareResult.ps1(1 hunks)source/Private/ConvertFrom-Reason.ps1(1 hunks)source/Private/Get-ClassName.ps1(1 hunks)source/Private/Resolve-Reason.ps1(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**
⚙️ 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
- Always update CHANGELOG.md Unreleased section
- Localize all strings using 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/Private/ConvertFrom-CompareResult.ps1CHANGELOG.mdsource/Private/Resolve-Reason.ps1source/Private/ConvertFrom-Reason.ps1source/Classes/010.ResourceBase.ps1source/Private/Get-ClassName.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: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.
- OUTPUTS: List each return type (one per line) with a 1‑line description. Must match both [OutputType()] and actual ...
Files:
source/Private/ConvertFrom-CompareResult.ps1source/Private/Resolve-Reason.ps1source/Private/ConvertFrom-Reason.ps1source/Classes/010.ResourceBase.ps1source/Private/Get-ClassName.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
- Format:
FunctionName_Description(underscore separators)- Example:
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/Private/ConvertFrom-CompareResult.ps1source/Private/Resolve-Reason.ps1source/Private/ConvertFrom-Reason.ps1source/Classes/010.ResourceBase.ps1source/Private/Get-ClassName.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)
- Surround fenced code blocks with blank lines
- Disable
MD013rule by adding a comment for tables/code blocks exceeding 80 characters
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 main changes as concise release notes
- 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
source/[cC]lasses/**/*.ps1
⚙️ CodeRabbit Configuration File
source/[cC]lasses/**/*.ps1: # DSC Class-Based Resource GuidelinesApplies to: Classes with
[DscResource(...)]decoration only.Requirements
- File:
source/Classes/{ResourceName}.ps1- Decoration:
[DscResource(RunAsCredential = 'Optional')](replace with'Mandatory'if required)- Inheritance: Must inherit
ResourceBase(part of module DscResource.Base)$this.localizedDatahashtable auto-populated byResourceBasefrom localization fileRequired Method Pattern
[MyResourceName] Get() { $currentState = ([ResourceBase] $this).Get() # If needed, post-processing based on returned current state before returning to user return $currentState } [System.Boolean] Test() { $inDesiredState = ([ResourceBase] $this).Test() # If needed, post-processing based on returned test result before returning to user return $inDesiredState } [void] Set() { ([ResourceBase] $this).Set() # If needed, additional state changes that could not be handled by Modify() } hidden [System.Collections.Hashtable] GetCurrentState([System.Collections.Hashtable] $properties) { # Return current state as hashtable # Variable $properties contains the key properties (key-value pairs). } hidden [void] Modify([System.Collections.Hashtable] $properties) { # Set desired state for non-compliant properties only # Variable $properties contains the properties (key-value pairs) that are not in desired state. } hidden [void] AssertProperties([System.Collections.Hashtable] $properties) { # Validate user-provided properties # Variable $properties contains properties user assigned values. } hidden [void] NormalizeProperties([System.Collections.Hashtable] $properties) { # Normalize user-provided properties # Variable $properties contains properties user assigned values. }
Files:
source/Classes/010.ResourceBase.ps1
🪛 LanguageTool
CHANGELOG.md
[grammar] ~16-~16: Use correct spacing
Context: ... of array addition and ForEach-Object. ### Removed - ResourceBase - Remove `Co...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
🔇 Additional comments (5)
source/Private/ConvertFrom-CompareResult.ps1 (1)
38-41: LGTM: explicit foreach improves performance and keeps pipeline behavior intactThe switch from ForEach-Object to foreach is correct. Parameter binding as [Hashtable[]] maintains behavior for both pipeline and direct invocation.
source/Private/Get-ClassName.ps1 (1)
46-46: LGTM: replace array accumulation with List[string]Using List.Add eliminates array reallocations while preserving the output type at the end. Good alignment with performance goals.
Also applies to: 51-51, 59-59
source/Private/ConvertFrom-Reason.ps1 (1)
40-40: LGTM: List[Hashtable] accumulator improves performance; return type preservedSwitching from += on arrays to List.Add() is correct and keeps the outward contract ([Hashtable[]]) unchanged.
Also applies to: 47-52
source/Private/Resolve-Reason.ps1 (2)
46-46: Good perf win: switch to List[Reason]Using System.Collections.Generic.List[Reason] avoids repeated array reallocations from += and aligns with the PR’s performance goals.
102-112: No change needed for the format string duplicationThe
'{0}:{0}:{1}'pattern is intentional and aligns with existing tests and theReasonclass’s setter behavior:
- ConvertFrom-Reason.Tests.ps1 asserts codes like
MyResource:MyResource:MyResourceProperty1.- Reason.Tests.ps1 uses
'{0}:{0}:Ensure' -f $mockReasonInstance.GetType()to validate the setter accepts the duplicate placeholder.No updates are required.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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)
tests/Unit/Classes/ResourceBase.Tests.ps1(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
- Always update CHANGELOG.md Unreleased section
- Localize all strings using 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:
tests/Unit/Classes/ResourceBase.Tests.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: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.
- OUTPUTS: List each return type (one per line) with a 1‑line description. Must match both [OutputType()] and actual ...
Files:
tests/Unit/Classes/ResourceBase.Tests.ps1
**/*.[Tt]ests.ps1
⚙️ CodeRabbit Configuration File
**/*.[Tt]ests.ps1: # Tests GuidelinesCore Requirements
- All public commands, private functions and classes must have unit tests
- All public commands and class-based resources must have integration tests
- Use Pester v5 syntax only
- One
Describeblock per file matching the tested entity name- Test code only inside
Describeblocks- Assertions only in
Itblocks- Never test
Write-Verbose,Write-Debug, or parameter binding behavior- Pass all mandatory parameters to avoid prompts
Structure & Scope
- Public commands: Never use
InModuleScope(unless retrieving localized strings)- Private functions/class resources: Always use
InModuleScope- Each scenario = separate
Contextblock- Use nested
Contextblocks for complex scenarios- Mocking in
BeforeAll(BeforeEachonly when required)- Setup/teardown in
BeforeAll,BeforeEach/AfterAll,AfterEachclose to usageSyntax Rules
- PascalCase:
Describe,Context,It,Should,BeforeAll,BeforeEach,AfterAll,AfterEachItdescriptions start with 'Should'Contextdescriptions start with 'When'- Mock variables prefix: 'mock'
- Prefer
-BeTrue/-BeFalseover-Be $true/-Be $false- No
Should -Not -Throw- invoke commands directlyFile Organization
- Class resources:
tests/Unit/Classes/{Name}.Tests.ps1- Public commands:
tests/Unit/Public/{Name}.Tests.ps1- Private functions:
tests/Unit/Private/{Name}.Tests.ps1Data-Driven Tests
- Define variables in separate
BeforeDiscoveryfor-ForEach(close to usage)-ForEachallowed onContextandItblocks- Keep scope close to usage context
Best Practices
- Assign unused return objects to
$null- Tested entity must be called from within the
Itblocks- Keep results and assertions in same
Itblock- Cover all scenarios and code paths
- Use
BeforeEachandAfterEachsparingly
Files:
tests/Unit/Classes/ResourceBase.Tests.ps1
🔇 Additional comments (2)
tests/Unit/Classes/ResourceBase.Tests.ps1 (2)
1247-1247: No change needed for whitespace.This extra newline is harmless and within the style guideline limits.
1248-1254: LGTM: Seeding PropertiesNotInDesiredState to drive Set() modify pathThis aligns with the refactor away from array addition and ensures the foreach-based path is exercised. Keys match what ConvertFrom-CompareResult consumes.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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)
source/Private/Resolve-Reason.ps1(4 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
- Always update CHANGELOG.md Unreleased section
- Localize all strings using 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/Private/Resolve-Reason.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: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.
- OUTPUTS: List each return type (one per line) with a 1‑line description. Must match both [OutputType()] and actual ...
Files:
source/Private/Resolve-Reason.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
- Format:
FunctionName_Description(underscore separators)- Example:
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/Private/Resolve-Reason.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). (7)
- GitHub Check: dsccommunity.DscResource.Base (Test HQRM)
- GitHub Check: dsccommunity.DscResource.Base (Test Integration WINDOWSPOWERSHELL_WIN2025)
- GitHub Check: dsccommunity.DscResource.Base (Test Integration POWERSHELL_WIN2025)
- GitHub Check: dsccommunity.DscResource.Base (Test Integration WINDOWSPOWERSHELL_WIN2022)
- GitHub Check: dsccommunity.DscResource.Base (Test Unit)
- GitHub Check: dsccommunity.DscResource.Base (Test Integration POWERSHELL_WIN2022)
- GitHub Check: dsccommunity.DscResource.Base (Build Package Module)
🔇 Additional comments (4)
source/Private/Resolve-Reason.ps1 (4)
3-3: SYNOPSIS grammar fix is correct.The change to “an array” reads correctly and matches the OutputType.
46-46: Good performance win: switch to List[Reason] avoids O(n²) array appends.Initializing a generic List and using Add() addresses the perf objective without altering outputs (still returning [Reason[]] via ToArray()).
118-118: Return type preserved.Returning
$reasons.ToArray()maintains the declared[Reason[]]OutputType. Matches the PR’s “no public API changes” objective.
102-112: Potential bug: duplicated ResourceName in Reason.CodeThe current code uses
Code = ('{0}:{0}:{1}' -f $ResourceName, $currentProperty.Property)which yields
<ResourceName>:<ResourceName>:<Property>. Unless you’re intentionally targeting a 3-part schema, this is likely incorrect.Please:
- Confirm the intended Reason.Code format.
- If you need a 2-part schema, change to:
- Code = ('{0}:{0}:{1}' -f $ResourceName, $currentProperty.Property) + Code = ('{0}:{1}' -f $ResourceName, $currentProperty.Property)- If you do require three segments, update the format string and document the schema.
- Add or update tests that assert the exact Reason.Code values to prevent regressions.
dan-hughes
force-pushed
the
performance-optimizations
branch
from
August 19, 2025 21:00
1344ca8 to
d61ff31
Compare
Member
|
@dan-hughes I will get back to reviewing soon. A lot at regular work and trying to get som work done in SqlServerDsc too. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
RequiredModules.psd1 (2)
11-16: Reconsider prerelease on runtime dep (DscResource.Common) for reproducible builds; add non-interactive install params.Allowing prerelease can pull unstable versions unexpectedly. If not strictly required, revert to stable. If it is required, at least add AcceptLicense/ErrorAction for CI reliability.
Option A — prefer stable:
- 'DscResource.Common' = @{ - Version = 'latest' - Parameters = @{ - AllowPrerelease = $true - } - } + 'DscResource.Common' = 'latest'Option B — keep prerelease but harden install:
'DscResource.Common' = @{ Version = 'latest' Parameters = @{ AllowPrerelease = $true + AcceptLicense = $true + ErrorAction = 'Stop' } }Please confirm whether a specific prerelease is needed; if yes, consider pinning with MinimumVersion instead of unconstrained “latest”.
28-33: OK to allow prerelease on test-only dep; add non-interactive flags.This is reasonable for DscResource.Test, but make CI deterministic/non-interactive.
'DscResource.Test' = @{ Version = 'latest' Parameters = @{ AllowPrerelease = $true + AcceptLicense = $true + ErrorAction = 'Stop' } }
📜 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)
RequiredModules.psd1(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**
⚙️ 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:
RequiredModules.psd1
**/*.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:
RequiredModules.psd1
⏰ 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). (7)
- GitHub Check: dsccommunity.DscResource.Base (Test Integration WINDOWSPOWERSHELL_WIN2025)
- GitHub Check: dsccommunity.DscResource.Base (Test Integration POWERSHELL_WIN2025)
- GitHub Check: dsccommunity.DscResource.Base (Test Integration WINDOWSPOWERSHELL_WIN2022)
- GitHub Check: dsccommunity.DscResource.Base (Test HQRM)
- GitHub Check: dsccommunity.DscResource.Base (Test Integration POWERSHELL_WIN2022)
- GitHub Check: dsccommunity.DscResource.Base (Test Unit)
- GitHub Check: dsccommunity.DscResource.Base (Build Package Module)
Contributor
Author
|
@johlju, can you circle back round to this? I'd like to test this with the changes in .Common to see if the the runtime improves. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
source/Private/Resolve-Reason.ps1 (1)
63-71: Enum branch fix uses the correct variable.
Switching to$currentProperty.ActualValueresolves the reference error; please ensure tests cover this branch.Run to confirm enum-branch coverage exists:
#!/bin/bash # Look for enum-focused tests in Resolve-Reason unit tests fd -a --glob 'Resolve-Reason*.Tests.ps1' tests || true rg -n -C2 --type=ps1 '\-is\s*\[System\.Enum\]' tests/Unit/Private/Resolve-Reason*.Tests.ps1 || true
🧹 Nitpick comments (1)
source/Private/Resolve-Reason.ps1 (1)
46-46: Prefer capacity preallocation when possible (minor perf).
If$Propertyis provided as an array (not via pipeline), initializing the list with a capacity avoids internal resizing.Example:
- $reasons = [System.Collections.Generic.List[Reason]]::new() + if ($PSBoundParameters.ContainsKey('Property') -and $Property) + { + $reasons = [System.Collections.Generic.List[Reason]]::new($Property.Count) + } + else + { + $reasons = [System.Collections.Generic.List[Reason]]::new() + }
📜 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/Get-ClassName.ps1(1 hunks)source/Private/Resolve-Reason.ps1(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- source/Private/Get-ClassName.ps1
🧰 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/Private/Resolve-Reason.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/Private/Resolve-Reason.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/Private/Resolve-Reason.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). (2)
- GitHub Check: dsccommunity.DscResource.Base (Build Package Module)
- GitHub Check: dsccommunity.DscResource.Base (Build Package Module)
🔇 Additional comments (1)
source/Private/Resolve-Reason.ps1 (1)
3-3: SYNOPSIS wording fix is correct.
Accurate and consistent with the function’s output type.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
source/Private/Resolve-Reason.ps1 (2)
63-71: Enum check fix is correct; add unit coverage for the enum branch.This resolves the variable reference issue. Please ensure tests cover ExpectedValue/ActualValue as [System.Enum] (match/mismatch).
#!/bin/bash # Locate Resolve-Reason tests and check for enum coverage set -euo pipefail file="$(fd -a --glob 'Resolve-Reason*.Tests.ps1' tests | head -n1 || true)" echo "Test file: ${file:-<not found>}" if [ -n "${file:-}" ]; then rg -n -C2 -i '(?-i:\[System\.Enum\])|\-is\s*\[System\.Enum\]' "$file" || echo "No explicit enum coverage found." fi
102-112: Localize user-facing Phrase and confirm Code format.Per repo localization guidelines, replace hardcoded English with a localized string key. Also, confirm duplicating
$ResourceNameinCode = ('{0}:{0}:{1}' ...)is intentional.Apply:
- $reasons.Add( - [Reason] @{ - Code = ('{0}:{0}:{1}' -f $ResourceName, $currentProperty.Property) - # Convert the object to JSON to handle complex types. - Phrase = ('The property {0} should be {1}, but was {2}' -f - $currentProperty.Property, - $propertyExpectedValueJson, - $propertyActualValueJson - ) - } - ) + $reasons.Add( + [Reason] @{ + Code = ('{0}:{0}:{1}' -f $ResourceName, $currentProperty.Property) + # Convert the object to JSON to handle complex types. + Phrase = ($script:localizedData.Resolve_Reason_PropertyShouldBeButWas -f + $currentProperty.Property, + $propertyExpectedValueJson, + $propertyActualValueJson + ) + } + )Add to strings file:
ConvertFrom-StringData @' Resolve_Reason_PropertyShouldBeButWas = The property {0} should be {1}, but was {2}. (RR0001) '@
🧹 Nitpick comments (1)
source/Private/Resolve-Reason.ps1 (1)
46-46: Good switch to List[Reason]; consider preallocating capacity when possible.Minor perf nit: when -Property is provided as an array (non‑pipeline), preallocate the list to reduce reallocations.
Example (outside the selected line range, in begin block):
# begin { $capacity = if ($PSBoundParameters.ContainsKey('Property') -and $Property) { $Property.Count } else { 0 } $reasons = [System.Collections.Generic.List[Reason]]::new($capacity)
📜 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)
source/Private/Resolve-Reason.ps1(4 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/Private/Resolve-Reason.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/Private/Resolve-Reason.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/Private/Resolve-Reason.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). (7)
- GitHub Check: dsccommunity.DscResource.Base (Test Integration WINDOWSPOWERSHELL_WIN2025)
- GitHub Check: dsccommunity.DscResource.Base (Test Integration POWERSHELL_WIN2025)
- GitHub Check: dsccommunity.DscResource.Base (Test Integration WINDOWSPOWERSHELL_WIN2022)
- GitHub Check: dsccommunity.DscResource.Base (Test HQRM)
- GitHub Check: dsccommunity.DscResource.Base (Test Unit)
- GitHub Check: dsccommunity.DscResource.Base (Test Integration POWERSHELL_WIN2022)
- GitHub Check: dsccommunity.DscResource.Base (Build Package Module)
🔇 Additional comments (2)
source/Private/Resolve-Reason.ps1 (2)
3-3: SYNOPSIS matches actual output ([Reason[]]).Accurate and consistent with OutputType.
118-118: Correct return type and shape.Returning
[Reason[]] $reasons.ToArray()avoids the unary-comma nested array pitfall and satisfies OutputType.
dan-hughes
changed the title
ResourceBase: Performance optimizations
johlju
approved these changes
Sep 24, 2025
Member
|
@dan-hughes looks good to me, ready to merge? |
johlju
added
the
ready for merge
Contributor
Author
|
Please, I think only WSManDsc is using this currently. |
johlju
removed
the
ready for merge
8 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Remove use of array addition and ForEach-Object
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