Remove Array addition in functions #173
Merged
Conversation
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
WalkthroughRefactors multiple scripts to replace array mutation via += with foreach-expression assignments for collecting items; adjusts tag/exclude-tag construction in Invoke-DscResourceTest similarly; updates HQRM build task collection; adds an Unreleased CHANGELOG entry documenting removal of array addition in listed functions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Script as "Script (function)"
participant Collector as "Collection (was accumulator)"
Note over Script,Collector #D3E4CD: Old flow (imperative accumulation)
Caller->>Script: call function
Script->>Collector: init empty array []
loop for each item
Script->>Collector: append item via +=
end
Script->>Caller: return collected array
Note over Script,Collector #FCE8D6: New flow (foreach-expression)
Caller->>Script: call function
Script->>Collector: evaluate foreach-expression (yields items)
Script->>Caller: return resulting array
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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 (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #173 +/- ##
=====================================
Coverage ? 80%
=====================================
Files ? 42
Lines ? 550
Branches ? 0
=====================================
Hits ? 443
Misses ? 107
Partials ? 0
🚀 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/Public/Invoke-DscResourceTest.ps1 (2)
374-389: Don’t overwrite existing TagFilter; merge with it and fix -notin RHS to array.Current logic replaces any existing TagFilter (e.g., from $Settings) with $newTag. Also, using a scalar RHS in -notin can enumerate string characters. Merge the sets and coerce RHS to arrays.
- $newTag = foreach ($optInTag in $optIns) + $newTag = foreach ($optInTag in $optIns) { - if ( - $optInTag -notin $PSBoundParameters['ExcludeTagFilter'] ` - -and $optInTag -notin $PSBoundParameters['TagFilter'] - ) + if ( + $optInTag -notin @($PSBoundParameters['ExcludeTagFilter']) -and + $optInTag -notin @($PSBoundParameters['TagFilter']) + ) { Write-Debug -Message "Adding tag $optInTag." - $optInTag + $optInTag } } - if ($newTag.Count -gt 0) - { - $PSBoundParameters['TagFilter'] = $newTag - } + if ($newTag) + { + $PSBoundParameters['TagFilter'] = @( + $PSBoundParameters['TagFilter'] + $newTag + ) | Where-Object { -not [string]::IsNullOrWhiteSpace($_) } | Select-Object -Unique + }
392-408: Same issue for ExcludeTagFilter; preserve existing and coerce RHS to arrays.Prevent clobbering an existing ExcludeTagFilter and avoid string RHS pitfalls with -notin.
- $newExcludeTag = foreach ($optOutTag in $optOuts) + $newExcludeTag = foreach ($optOutTag in $optOuts) { - if ( - $optOutTag -notin $PSBoundParameters['TagFilter'] ` - -and $optOutTag -notin $PSBoundParameters['ExcludeTagFilter'] - ) + if ( + $optOutTag -notin @($PSBoundParameters['TagFilter']) -and + $optOutTag -notin @($PSBoundParameters['ExcludeTagFilter']) + ) { Write-Debug -Message "Adding ExcludeTag $optOutTag." - $optOutTag + $optOutTag } } - if ($newExcludeTag.Count -gt 0) - { - $PSBoundParameters['ExcludeTagFilter'] = $newExcludeTag - } + if ($newExcludeTag) + { + $PSBoundParameters['ExcludeTagFilter'] = @( + $PSBoundParameters['ExcludeTagFilter'] + $newExcludeTag + ) | Where-Object { -not [string]::IsNullOrWhiteSpace($_) } | Select-Object -Unique + }
🧹 Nitpick comments (3)
CHANGELOG.md (1)
56-61: Tighten wording and file name in Changed entry.Grammar nit and a filename detail; propose small edit.
- - Remove array addition in following Public/Private functions. + - Remove array addition in the following functions and build task. - `Get-ClassResourceNameFromFile` - `Get-ModuleScriptResourceName` - `Get-SuppressedPSSARuleNameList` - `Invoke-DscResourceTest` - - `Invoke_HQRM_Tests.build` + - `Invoke_HQRM_Tests.build.ps1`source/tasks/Invoke_HQRM_Tests.build.ps1 (1)
425-428: Good swap to foreach-expression; consider ensuring a stable, non-null container list.Current code can yield $null when no files are found; also ordering may vary by filesystem. Optional guard + deterministic order below.
- $pesterContainers = foreach ($testScript in $hqrmTestScripts) + $pesterContainers = foreach ($testScript in $hqrmTestScripts) { New-PesterContainer -Path $testScript.FullName -Data $pesterData.Clone() } + # Ensure array type even when no scripts are found. + $pesterContainers = @($pesterContainers)Outside the changed hunk (context):
# Prefer deterministic order and files only. $hqrmTestScripts = Get-ChildItem -Path $pathToHqrmTests -File -Filter '*.Tests.ps1' | Sort-Object -Property FullNamesource/Private/Get-ClassResourceNameFromFile.ps1 (1)
32-37: Preserve consistent output type for empty matches.Foreach-expression can yield $null when no classes match. Coerce to an empty string array to align with [OutputType([String[]])].
$classResourceNames = foreach ($typeDefinitionAst in $typeDefinitionAsts) { if ($typeDefinitionAst.Attributes.TypeName.Name -ieq 'DscResource') { $typeDefinitionAst.Name } } + # Ensure consistent array output for callers relying on enumeration or .Count. + $classResourceNames = @([string[]] $classResourceNames)
📜 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 (6)
CHANGELOG.md(1 hunks)source/Private/Get-ClassResourceNameFromFile.ps1(1 hunks)source/Private/Get-ModuleScriptResourceName.ps1(1 hunks)source/Private/Get-SuppressedPSSARuleNameList.ps1(1 hunks)source/Public/Invoke-DscResourceTest.ps1(3 hunks)source/tasks/Invoke_HQRM_Tests.build.ps1(1 hunks)
🧰 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 in PowerShell, from repository root
- Build before running tests:
.\build.ps1 -Tasks build- Always run tests in new PowerShell session:
Invoke-Pester -Path @({test paths}) -Output DetailedFile 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/Get-ModuleScriptResourceName.ps1source/Private/Get-ClassResourceNameFromFile.ps1CHANGELOG.mdsource/tasks/Invoke_HQRM_Tests.build.ps1source/Private/Get-SuppressedPSSARuleNameList.ps1source/Public/Invoke-DscResourceTest.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:
@{}- 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: Lis...
Files:
source/Private/Get-ModuleScriptResourceName.ps1source/Private/Get-ClassResourceNameFromFile.ps1source/tasks/Invoke_HQRM_Tests.build.ps1source/Private/Get-SuppressedPSSARuleNameList.ps1source/Public/Invoke-DscResourceTest.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/{MyModuleName}.strings.psd1- Class resources:
source/en-US/{ResourceClassName}.strings.psd1Key Naming Patterns
- Format:
Verb_FunctionName_Action(underscore separators), e.g.Get_Database_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/Get-ModuleScriptResourceName.ps1source/Private/Get-ClassResourceNameFromFile.ps1source/tasks/Invoke_HQRM_Tests.build.ps1source/Private/Get-SuppressedPSSARuleNameList.ps1source/Public/Invoke-DscResourceTest.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 #<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
🧠 Learnings (2)
📓 Common learnings
Learnt from: johlju
PR: dsccommunity/DscResource.Test#167
File: source/Private/Test-FileContainsClassResource.ps1:44-56
Timestamp: 2025-08-09T19:29:36.323Z
Learning: In the DscResource.Test repository, DSC resource attributes are consistently written as `[DscResource(...)]` rather than using variations like `[DscResourceAttribute()]` or fully qualified names like `[Microsoft.PowerShell.DesiredStateConfiguration.DscResource()]`. The Test-FileContainsClassResource function should focus on detecting the standard `[DscResource(...)]` pattern that is actually used in the codebase.
📚 Learning: 2025-08-09T19:29:36.323Z
Learnt from: johlju
PR: dsccommunity/DscResource.Test#167
File: source/Private/Test-FileContainsClassResource.ps1:44-56
Timestamp: 2025-08-09T19:29:36.323Z
Learning: In the DscResource.Test repository, DSC resource attributes are consistently written as `[DscResource(...)]` rather than using variations like `[DscResourceAttribute()]` or fully qualified names like `[Microsoft.PowerShell.DesiredStateConfiguration.DscResource()]`. The Test-FileContainsClassResource function should focus on detecting the standard `[DscResource(...)]` pattern that is actually used in the codebase.
Applied to files:
source/Private/Get-ClassResourceNameFromFile.ps1CHANGELOG.mdsource/Public/Invoke-DscResourceTest.ps1
🪛 LanguageTool
CHANGELOG.md
[grammar] ~56-~56: There might be a mistake here.
Context: ...tAnalyzer rule. - Remove array addition in following Public/Private functions. -...
(QB_NEW_EN)
johlju
approved these changes
Sep 1, 2025
Member
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju reviewed 5 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-hughes)
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
Improve performance by not using Array addition operation.
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed and how that affects users (if applicable), and
reference the issue being resolved (if applicable).
This change is