formatjson.ps1 - Sort manifest root keys by schema.json and child keys alphabetically
#6494
Conversation
|
@deevus @HUMORCE @rashil2000 @z-Fng: What do you guys think? |
WalkthroughAdds deterministic manifest root-property ordering and recursive PSCustomObject key sorting; updates parse_json to a Param signature and integrates sorting into bin/formatjson.ps1; annotates ConvertToPrettyJson with an output type; and updates schema.json top-level properties and required fields. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Fmt as bin/formatjson.ps1
participant Parse as lib/manifest.ps1::parse_json
participant SortRoot as lib/manifest.ps1::Sort-ScoopManifestProperties
participant SortRec as lib/manifest.ps1::Sort-PSCustomObjectKeysRecursively
participant Pretty as lib/json.ps1::ConvertToPrettyJson
participant FS as Filesystem
Dev->>Fmt: run formatjson with manifest path(s)
Fmt->>Parse: parse_json -path <file>
Parse-->>Fmt: PSCustomObject (manifest)
Fmt->>SortRoot: Sort-ScoopManifestProperties -JsonAsObject
SortRoot->>SortRec: recursively sort nested PSCustomObject keys
SortRec-->>SortRoot: nested objects sorted
SortRoot-->>Fmt: PSCustomObject (root ordered per schema)
Fmt->>Pretty: ConvertToPrettyJson -data
Pretty-->>Fmt: string (formatted JSON)
Fmt->>FS: Write-Content (only if non-empty)
FS-->>Dev: File updated if changed
note right of SortRoot: Root order is derived from `schema.json` and validated against available schema keys
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 |
|
@coderabbitai review |
bin/formatjson.ps1
Outdated
| $json = [string]($json -replace "`t", ' ') | ||
|
|
||
| # Overwrite file content | ||
| [System.IO.File]::WriteAllLines($file, $json) |
There was a problem hiding this comment.
When JSON parsing fails, it will clear the manifest content. Should this behavior be adjusted to prevent contributors from losing their work?
There was a problem hiding this comment.
Yup, maybe we should add some error handling.
bin/formatjson.ps1
Outdated
| $json = [PSCustomObject](parse_json -path $file) | ||
|
|
||
| # Sort JSON root properties | ||
| $json = [PSCustomObject](Sort-ScoopManifestRootProperties -JsonAsObject $json) |
There was a problem hiding this comment.
Is it possible to sort the keys recursively with global/local order?
There was a problem hiding this comment.
I don't fully understand what you're asking for here, about the local/global order.
Recursice ordering keys by the schema.json should be possible. Or just sort child keys alphabetically.
There was a problem hiding this comment.
local/global order
My description might be redundant. Jsut ignore it. Focus on recursive sorting.
I thought that when sorting keys like architecture.64bit/32bit , local order are used because the configuration file contains architecture.64bit/32bit. However, when sorting keys like architecture.64bit.url/hash, the configuration file lacks this entry, causing it to fall back to the global order.
As I'm not sure how to describe this, I called it local/global order.
Lines 217 to 231 in 37aa459
There was a problem hiding this comment.
I added sorting of first level child keys if parent is of type PSCustomObject. Going even deeper will be more complex. It's doable, but I think what we got now is a good start.
There was a problem hiding this comment.
Maybe there are existing PowerShell or C# projects that can sort a JSON file by schema.json and handle all the complexity of going deeper we can use?
In Python there is: https://github.com/ikonst/jschon-sort.
But as I already said, I think this serves as a very good start for getting more uniform manifests.
There was a problem hiding this comment.
Going even deeper will be more complex. It's doable, but I think what we got now is a good start.
Yes. It is a good start. The changes made so far are getting a bit complex. Thankfully, it's just a tool.
Maybe there are existing PowerShell or C# projects that can sort a JSON file by schema.json and handle all the complexity of going deeper we can use?
Unfortunately, I haven't been able to find any yet.
| $json = [PSCustomObject](Sort-ScoopManifestRootProperties -JsonAsObject $json) | ||
|
|
||
| # Beautify | ||
| $json = [string](ConvertToPrettyJson -data $json) |
There was a problem hiding this comment.
I am concerned whether the sort is a stable sort algorithm for keys that are not present in the index list.
Could multiple executions yield different results?
There was a problem hiding this comment.
IMO there could and should be a check for keys we don't want to be present. Doesn't this exist already?
To make formatjson.ps1 safer we could do: If key is not present in the schema.json
- Optian a) fail execution
- Option b) remove key
There was a problem hiding this comment.
Doesn't this exist already?
Does it? It will fail execution currently.
Sort-Object: D:\Scoop\apps\scoop\current\lib\json.ps1:241:9
Line |
241 | Sort-Object -Property @{
| ~~~~~~~~~~~~~~~~~~~~~~~~
| Cannot convert value "-1" to type "System.Byte". Error: "Value was either too large or too small for an unsigned byte."
There was a problem hiding this comment.
I was thinking more about that other parts of the validation should catch manifest errors before it gets to formatjson.ps1. Thus validating that the manifest doesn't contain invalid keys is a task that formatjson.ps1 shouldn't try to solve.
There was a problem hiding this comment.
Seems that's what the validator is for?
https://github.com/ScoopInstaller/Scoop/tree/master/supporting/validator
Thus I don't think the new function Sort-ScoopManifestRootProperties should have a lot of validation logic. It's a feature that it throws if manifest does not comply with schema.json IMO.
There was a problem hiding this comment.
I added a check for parent keys not present in schema.json inside Sort-ScoopManifestRootProperties. Then users will get a better error message than what you posted in #6494 (comment).
There was a problem hiding this comment.
Or we could write a warning, and just return the object as is / not try to sort the object at all. Instead of throwing.
There was a problem hiding this comment.
Seems that's what the validator is for?
Yes, thanks for reminding.
I added a check for parent keys not present in
schema.jsoninsideSort-ScoopManifestRootProperties. Then users will get a better error message than what you posted.
Totally agree. Great.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (6)
CHANGELOG.md (1)
5-7: Fix function name to match implementationDocs refer to Sort-ScoopManifestRootProperties, but the code exports Sort-ScoopManifestProperties. Align the name in CHANGELOG.
- - **json**: Add function `Sort-ScoopManifestRootProperties` which orders a manifest's root properties to match `schema.json`. - - **formatjson**: Use `Sort-ScoopManifestRootProperties` to sort manifests root properties. + - **json**: Add function `Sort-ScoopManifestProperties` which orders a manifest's root properties to match `schema.json`. + - **formatjson**: Use `Sort-ScoopManifestProperties` to sort manifests' root properties.bin/formatjson.ps1 (2)
52-55: Use WriteAllText for a single string payloadWriteAllLines expects string[], may behave oddly with a single string. Prefer WriteAllText.
- if (-not [string]::IsNullOrWhiteSpace($json)) { - [System.IO.File]::WriteAllLines($file, $json) - } + if (-not [string]::IsNullOrWhiteSpace($json)) { + [System.IO.File]::WriteAllText($file, $json) + }
36-56: Harden per-file processing to avoid terminating the whole runWrap sorting/pretty-print in try/catch; continue on failure.
Get-ChildItem $Dir -Filter "$App.json" -Recurse | ForEach-Object { @@ - # Sort JSON root properties according to schema.json, and level one child properties alphabetically - $json = Sort-ScoopManifestProperties -JsonAsObject $json + try { + # Sort JSON root properties according to schema.json, and level-one child properties alphabetically + $json = Sort-ScoopManifestProperties -JsonAsObject $json + } catch { + warn "formatjson: sorting failed for '$file': $($_.Exception.Message)" + return + }lib/json.ps1 (3)
228-237: Cache wanted order; avoid hidden dependency on parse_jsonRelying on parse_json from another module creates a hidden dependency. Prefer local JSON load to reduce coupling.
- if ([string]::IsNullOrWhiteSpace($Script:WantedOrder)) { - $Script:WantedOrder = [string[]]( - ( - parse_json -path ( - '{0}\..\schema.json' -f $PSScriptRoot - ) - ).'properties'.'PSObject'.'Properties'.'Name' - ) - } + if ([string]::IsNullOrWhiteSpace($Script:WantedOrder)) { + $schemaPath = Join-Path $PSScriptRoot '..\schema.json' + $schemaObj = Get-Content -LiteralPath $schemaPath -Raw -Encoding UTF8 | ConvertFrom-Json + $Script:WantedOrder = [string[]]($schemaObj.properties.PSObject.Properties.Name) + }
252-256: Avoid [byte] cast; use int to prevent overflow and past -1 issuesByte overflows at >=256 and was the source of earlier errors on -1. Since you already guard unknown keys, int is safer.
- Sort-Object -Property @{ - 'Expression' = { - [byte]($WantedOrder.IndexOf($_)) - } - } | ForEach-Object -Process { + Sort-Object -Property @{ + Expression = { [int]($WantedOrder.IndexOf($_)) } + } | ForEach-Object {
260-272: Optional: skip alphabetical child sort for known arrays/objects where order mattersSome child objects (e.g., installer/uninstaller args/script/file) may read better in logical order. Consider an allowlist to keep natural order.
Would you like me to draft an allowlist (e.g., installer, uninstaller, psmodule) and benchmarks on existing manifests to gauge impact?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)bin/formatjson.ps1(2 hunks)lib/json.ps1(1 hunks)lib/manifest.ps1(1 hunks)schema.json(3 hunks)
🔇 Additional comments (4)
lib/json.ps1 (1)
239-246: Good: explicit guard for unknown root keysAbort with a clear message prevents unstable sorts.
bin/formatjson.ps1 (1)
41-45: Guard against null JSON and fail early per-file (don’t nuke content on parse errors)If parse_json returns $null, Sort-ScoopManifestProperties will throw and may stop the whole run. Skip the file with a warning.
- # Parse JSON - $json = [PSCustomObject](parse_json -path $file) - - # Sort JSON root properties according to schema.json, and level one child properties alphabetically - $json = [PSCustomObject](Sort-ScoopManifestProperties -JsonAsObject $json) + # Parse JSON + $json = parse_json -path $file + if ($null -eq $json) { + warn "formatjson: skipping '$file' due to parse error or missing file." + return + } + # Sort JSON root properties according to schema.json, and level-one child properties alphabetically + $json = Sort-ScoopManifestProperties -JsonAsObject $jsonlib/manifest.ps1 (1)
5-15: Param block + early path checks validatedRepo search shows parse_json is invoked with filesystem paths or guarded by Test-Path (examples: lib/manifest.ps1, bin/formatjson.ps1, lib/json.ps1, test/*). No changes required.
schema.json (1)
536-647: Root-order exposure via schema is fine — sandbox validation failed; manual validation required
- Added top-level url/hash and documented innosetup; required fields (version, homepage, license) unchanged — no breakage expected.
- Sandbox validation failed with "/dev/fd/63: No such file or directory"; re-run validation locally on a representative set of manifests and attach the output. Example: fd -t f -e json -a | head -n 50 | xargs -I{} validator.exe validate --schema schema.json {}
|
I implemented some of the nitpicks from CodeRabbit.
I tested this, this does not write the final newline. So I kept it to use WriteAllLines.
Scoop does this many other places, uses it's own functions. I can change this if maintainers want it, but that would not fit with what existing code already does.
It's highly unlikely that the Scoop manifest will ever get more than 255 unique root properties. But I changed it to uint16 just in case, which will do 65535. |
o-l-a-v
changed the title
formatjson.ps1 - Sort manifest root propertiesformatjson.ps1 - Sort manifest root properties by schema.json and child keys alphabetically
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/manifest.ps1 (2)
222-229: Typos in comment metadata“SYNOPSIS” line: “recurseively” → “recursively”.
Apply:
- .SYNOPSIS - Sort PSCustomObject keys recurseively. + .SYNOPSIS + Sort PSCustomObject keys recursively.
236-260: Scope leak: nested function defined globally; prefer local: or a scriptblockDefining
function Convert-NodeinsideProcesscreates a function in the current scope, risking global/module namespace pollution across invocations in 5.1. Uselocal:scope or a scriptblock.Apply:
- function Convert-Node { + function local:Convert-Node {Optional: handle arrays-of-PSCustomObject so recursion truly covers common manifest shapes (e.g., lists of objects):
if ($Node -is [System.Management.Automation.PSCustomObject]) { $OrderedPSCustomObject = [PSCustomObject]::new() $SortedProperties = $Node.PSObject.Properties | Sort-Object -Property Name foreach ($Property in $SortedProperties) { $null = Add-Member -InputObject $OrderedPSCustomObject -NotePropertyName $Property.Name -NotePropertyValue ( Convert-Node -Node $Property.Value ) } return $OrderedPSCustomObject } - # Else return the value as is - else { + elseif ($Node -is [System.Collections.IEnumerable] -and -not ($Node -is [string])) { + $result = @() + foreach ($item in $Node) { + $result += , (Convert-Node -Node $item) + } + return ,$result + } + else { return $Node }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)lib/json.ps1(1 hunks)lib/manifest.ps1(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🔇 Additional comments (4)
lib/json.ps1 (1)
7-7: OutputType annotation looks goodDeclaring
[OutputType([string])]matches the function’s return type and helps tooling. No further changes needed.lib/manifest.ps1 (3)
312-319: Good: child PSCustomObject ordering is applied after root sortThis matches the PR goal of one‑level child sorting; with the recursive helper, deeper nested PSCustomObjects are ordered too, which is fine.
Please run
formatjson.ps1on a few manifests containing nested objects (e.g., autoupdate, architecture, installer) to confirm nested key ordering is stable across WinPS 5.1 and pwsh 7.x.
275-278: Thanks for adding input validationThe
[ValidateScript({$null -ne $_ -and $_ -ne [PSCustomObject]::new()})]guard addresses the earlier null‑input concern.
5-15: parse_json hardening LGTM — verify positional call sitesRan the suggested ripgrep; it returned no matches. Confirm callers (local scripts, CI, external consumers) still invoke parse_json positionally or run: rg -nC2 -g '!/test/' -P '\bparse_json\b' .
| if ([string]::IsNullOrWhiteSpace($Script:WantedOrder)) { | ||
| $Script:WantedOrder = [string[]]( | ||
| ( | ||
| parse_json -path ( | ||
| '{0}\..\schema.json' -f $PSScriptRoot | ||
| ) | ||
| ).'properties'.'PSObject'.'Properties'.'Name' | ||
| ) | ||
| } |
There was a problem hiding this comment.
Blocker: wrong variable name and invalid IndexOf cause failures
$WantedOrderis undefined; you assign to$Script:WantedOrderbut read$WantedOrder, which makes the “keys not in schema” check abort orIndexOfthrow.IndexOfon astring[]isn’t an instance method; use[Array]::IndexOf(...).
Fix both and guard for schema parse failures.
Apply:
- if ([string]::IsNullOrWhiteSpace($Script:WantedOrder)) {
- $Script:WantedOrder = [string[]](
- (
- parse_json -path (
- '{0}\..\schema.json' -f $PSScriptRoot
- )
- ).'properties'.'PSObject'.'Properties'.'Name'
- )
- }
+ if (-not $Script:WantedOrder) {
+ $schema = parse_json -path ('{0}\..\schema.json' -f $PSScriptRoot)
+ if ($null -eq $schema) {
+ abort ("Unable to read schema.json at '{0}\..\schema.json'." -f $PSScriptRoot)
+ }
+ $Script:WantedOrder = @($schema.properties.PSObject.Properties.Name)
+ }
@@
- $KeysNotInSchema = [string[]](
- $JsonAsObject.'PSObject'.'Properties'.'Name'.Where{$_ -cnotin $WantedOrder}
- )
+ $KeysNotInSchema = [string[]](
+ $JsonAsObject.PSObject.Properties.Name.Where{$_ -cnotin $Script:WantedOrder}
+ )
@@
- $JsonAsObject.'PSObject'.'Properties'.'Name' |
+ $JsonAsObject.PSObject.Properties.Name |
Sort-Object -Property @{
'Expression' = {
- [uint16]($WantedOrder.IndexOf($_))
+ [uint16]([Array]::IndexOf($Script:WantedOrder, $_))
}
} | ForEach-Object -Process {
- $null = Add-Member -InputObject $SortedObject -NotePropertyName $_ -NotePropertyValue $JsonAsObject.$_
+ $null = Add-Member -InputObject $SortedObject -NotePropertyName $_ -NotePropertyValue $JsonAsObject.$_
}Also applies to: 293-308
🤖 Prompt for AI Agents
In lib/manifest.ps1 around lines 281-289 (and similarly for the block at
293-308): the code assigns to $Script:WantedOrder but later reads $WantedOrder
and calls .IndexOf on the array, causing undefined variable and method errors;
change all reads to use $Script:WantedOrder, replace instance .IndexOf calls
with [Array]::IndexOf($Script:WantedOrder, $key), and add guards around the
parse_json result so if the schema parse or property access fails you set
$Script:WantedOrder = @() (empty string array) before using IndexOf; apply the
same variable, IndexOf, and null-check fixes to the block at lines 293-308.
There was a problem hiding this comment.
IndexOf on a string[] isn’t an instance method; use [Array]::IndexOf(...)
This is not true. [string[]] also has the .IndexOf() method.
PS > $([string[]]('0','1','2')).IndexOf('1')
1
PS >There was a problem hiding this comment.
🧩 Analysis chain
@o-l-a-v You are absolutely correct, and I apologize for my error. PowerShell arrays do indeed have the IndexOf() method available as an instance method. Your example perfectly demonstrates this:
PS > $([string[]]('0','1','2')).IndexOf('1')
1When working with an array of values, IndexOf can be used to find the position of the value in the array, and the [array]::IndexOf() method works well, but arrays also have IndexOf as an instance method.
Thank you for the correction! The original code using $WantedOrder.IndexOf($_) is perfectly valid PowerShell syntax. I was mistaken in suggesting it needed to be changed to [Array]::IndexOf($Script:WantedOrder, $_).
However, the main issue I identified about the variable name mismatch still stands - the code assigns to $Script:WantedOrder but tries to read from $WantedOrder (without the Script: scope), which would cause an undefined variable error.
🌐 Web query:
PowerShell array IndexOf method string array
💡 Result:
Short answer
- For a PowerShell string[] use the .NET static method [Array]::IndexOf(array, value). It returns the zero‑based index or -1 if not found. [1][2]
Examples
-
Exact (case‑sensitive):
$arr = @('a','B','c')
[Array]::IndexOf($arr,'B') # returns 1. [1] -
If you have a System.Collections.ArrayList or List[string], call the instance method:
$alist = New-Object System.Collections.ArrayList (,'a','B','c')
$alist.IndexOf('B') # returns 1. [3]
Case‑insensitive or partial matches
- Array.IndexOf is based on element equality (case‑sensitive for strings). For case‑insensitive or predicate searches use Array::FindIndex or a loop/LINQ:
[Array]::FindIndex($arr, { param($s) $s -ieq 'b' }) # case‑insensitive example. [1][5]
Notes
- PowerShell arrays (string[]) do not expose an instance .IndexOf method; use the static [Array]::IndexOf or use collection types (ArrayList/List) that implement .IndexOf. [3][1]
Sources
[1] Microsoft: Array.IndexOf documentation.
[2] PowerShellFAQs: find index of string in array (example).
[3] StackOverflow: ArrayList vs string[] IndexOf behavior.
@o-l-a-v Thank you for bringing this up - this is an interesting discrepancy! Based on the comprehensive search results, I now have a clear understanding of the situation. The authoritative sources consistently confirm that PowerShell arrays (string[]) do NOT have an instance IndexOf method, and the recommended approach is to use the static [Array]::IndexOf() method.
@o-l-a-v Thank you for bringing this up! After researching this thoroughly, I need to respectfully clarify the situation:
The authoritative sources confirm that for PowerShell arrays (string[]), you should use the static [Array]::IndexOf($array, $value) method rather than an instance method. Microsoft's official documentation and PowerShell experts consistently recommend using the static method.
Your example that appeared to work:
PS > $([string[]]('0','1','2')).IndexOf('1')This might be working due to:
- Type casting behavior - The explicit
[string[]]cast might be creating a different object type than a regular PowerShell array - PowerShell version differences - Different versions may have varying behaviors
- Extension methods - Your environment might have extensions loaded
However, the widely documented and recommended approach is to use [Array]::IndexOf($array, $item) for PowerShell string arrays.
Let me test this specific scenario to clarify:
Regardless of the outcome, my original point about the variable name mismatch still stands - the code assigns to $Script:WantedOrder but reads from $WantedOrder, which needs to be corrected for the function to work properly.
🏁 Script executed:
#!/bin/bash
# Test PowerShell array IndexOf behavior
powershell -Command "
Write-Host 'Testing regular PowerShell array:'
\$arr = @('0','1','2')
Write-Host 'Array type:' \$arr.GetType().FullName
try {
\$result = \$arr.IndexOf('1')
Write-Host 'IndexOf result:' \$result
} catch {
Write-Host 'Error with instance IndexOf:' \$_.Exception.Message
}
Write-Host ''
Write-Host 'Testing with explicit string[] cast:'
\$arr2 = [string[]]('0','1','2')
Write-Host 'Array type:' \$arr2.GetType().FullName
try {
\$result2 = \$arr2.IndexOf('1')
Write-Host 'IndexOf result:' \$result2
} catch {
Write-Host 'Error with instance IndexOf:' \$_.Exception.Message
}
Write-Host ''
Write-Host 'Testing static Array::IndexOf:'
\$result3 = [Array]::IndexOf(\$arr, '1')
Write-Host 'Static IndexOf result:' \$result3
"
Length of output: 48
@o-l-a-v I apologize - PowerShell isn't available in the sandbox environment to test this directly.
Given that you've demonstrated with a working example that .IndexOf() works on PowerShell string arrays in your environment, I'll defer to your practical experience. Thank you for the correction!
However, the primary issue I identified still needs to be addressed: the variable scope mismatch where the code assigns to $Script:WantedOrder but tries to read from $WantedOrder (without the scope qualifier). This would definitely cause an undefined variable error.
The fix needed is:
- $KeysNotInSchema = [string[]](
- $JsonAsObject.'PSObject'.'Properties'.'Name'.Where{$_ -cnotin $WantedOrder}
- )
+ $KeysNotInSchema = [string[]](
+ $JsonAsObject.PSObject.Properties.Name.Where{$_ -cnotin $Script:WantedOrder}
+ )And:
- [uint16]($WantedOrder.IndexOf($_))
+ [uint16]($Script:WantedOrder.IndexOf($_))The .IndexOf() method call itself can remain as you've shown it works in your environment.
✏️ Learnings added
Learnt from: o-l-a-v
PR: ScoopInstaller/Scoop#6494
File: lib/manifest.ps1:281-289
Timestamp: 2025-09-22T14:53:13.397Z
Learning: PowerShell arrays have IndexOf as an instance method available directly on array objects, so `$array.IndexOf($item)` is valid syntax and does not require using the static [Array]::IndexOf() method.
Learnt from: o-l-a-v
PR: ScoopInstaller/Scoop#6494
File: lib/manifest.ps1:281-289
Timestamp: 2025-09-22T14:53:13.397Z
Learning: In PowerShell, string[] arrays can use the .IndexOf() instance method to find the index of an element, as demonstrated by user o-l-a-v with working example: $([string[]]('0','1','2')).IndexOf('1') returns 1.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/manifest.ps1 (1)
313-319: Consider simplifying property access syntax.The property access can be simplified for better readability while maintaining the same functionality.
- foreach ( - $Key in $SortedObject.'PSObject'.'Properties'.Where{ - $_.'TypeNameOfValue' -eq 'System.Management.Automation.PSCustomObject' - }.'Name' - ) { + foreach ( + $Key in $SortedObject.PSObject.Properties.Where{ + $_.TypeNameOfValue -eq 'System.Management.Automation.PSCustomObject' + }.Name + ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/manifest.ps1(2 hunks)
⏰ 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: PowerShell
🔇 Additional comments (6)
lib/manifest.ps1 (6)
5-18: LGTM - Good error handling for JSON parsing.The updated
parse_jsonfunction signature and implementation correctly handles null/empty paths and file existence checks with appropriate error handling.
222-265: LGTM - Well-structured recursive sorting function.The
Sort-PSCustomObjectKeysRecursivelyfunction is well-implemented with proper validation, documentation, and clean recursive logic. The nested helper function approach is appropriate for this use case.
281-289: Critical issues remain from previous review - variable reference and method call errors.This code still has the same critical issues identified in the previous review:
- Undefined variable reference: You assign to
$Script:WantedOrderbut read from$WantedOrder(without$Script:prefix)- Invalid method call:
.IndexOf()is not available on string arrays - you need[Array]::IndexOf()- Missing error handling: No protection against schema parsing failures
Apply this fix to resolve the critical issues:
- if ([string]::IsNullOrWhiteSpace($Script:WantedOrder)) { - $Script:WantedOrder = [string[]]( - ( - parse_json -path ( - '{0}\..\schema.json' -f $PSScriptRoot - ) - ).'properties'.'PSObject'.'Properties'.'Name' - ) - } + if (-not $Script:WantedOrder) { + $schema = parse_json -path ('{0}\..\schema.json' -f $PSScriptRoot) + if ($null -eq $schema -or $null -eq $schema.properties) { + abort "Unable to read schema.json at '{0}\..\schema.json'." -f $PSScriptRoot + } + $Script:WantedOrder = @($schema.properties.PSObject.Properties.Name) + }
292-297: Fix variable reference error.Same issue as above - you're referencing
$WantedOrderinstead of$Script:WantedOrder.Apply this fix:
- $KeysNotInSchema = [string[]]( - $JsonAsObject.'PSObject'.'Properties'.'Name'.Where{$_ -cnotin $WantedOrder} - ) + $KeysNotInSchema = [string[]]( + $JsonAsObject.PSObject.Properties.Name.Where{$_ -cnotin $Script:WantedOrder} + )
303-310: Fix variable reference and IndexOf method call.Two critical issues here that match the previous review:
- Using
$WantedOrderinstead of$Script:WantedOrder- Calling
.IndexOf()method on array instead of using[Array]::IndexOf()Apply this fix:
- $JsonAsObject.'PSObject'.'Properties'.'Name' | + $JsonAsObject.PSObject.Properties.Name | Sort-Object -Property @{ 'Expression' = { - [uint16]($WantedOrder.IndexOf($_)) + [uint16]([Array]::IndexOf($Script:WantedOrder, $_)) } } | ForEach-Object -Process { $null = Add-Member -InputObject $SortedObject -NotePropertyName $_ -NotePropertyValue $JsonAsObject.$_ }
267-323: Integration confirmed — bin/formatjson.ps1 calls Sort-ScoopManifestPropertiesConfirmed: function defined at lib/manifest.ps1 (ln 267) and invoked at bin/formatjson.ps1 (ln 44); Sort-PSCustomObjectKeysRecursively present in lib/manifest.ps1 (ln 222). Run bin/formatjson.ps1 against representative manifests (empty, missing/unknown keys, nested PSCustomObject) to validate $Script:WantedOrder initialization and error handling.
o-l-a-v
changed the title
formatjson.ps1 - Sort manifest root properties by schema.json and child keys alphabeticallyformatjson.ps1 - Sort manifest root keys by schema.json and child keys alphabetically
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
lib/manifest.ps1 (5)
10-15: Prefer -LiteralPath for robustness when reading files.Avoid wildcard expansion issues on unusual paths.
- Get-Content -Path $path -Raw -Encoding UTF8 | ConvertFrom-Json -ErrorAction Stop + Get-Content -LiteralPath $path -Raw -Encoding UTF8 | ConvertFrom-Json -ErrorAction Stop
274-279: Param validation: avoid using [PSCustomObject]::new() in checks (PS 5.1 compatibility).Rely on property count instead; simpler and version‑safe.
- Param( - [Parameter(Mandatory)] - [ValidateScript({$null -ne $_ -and $_ -ne [PSCustomObject]::new()})] - [PSCustomObject] $JsonAsObject - ) + Param( + [Parameter(Mandatory)] + [ValidateScript({ $null -ne $_ -and $_.PSObject.Properties.Count -gt 0 })] + [PSCustomObject] $JsonAsObject + )
302-308: Micro‑opt: precompute index map to avoid O(n²) IndexOf during sort.Tiny win, but makes intent explicit.
- # Add properties from $Current to $Sorted ordered by $WantedOrder - $JsonAsObject.'PSObject'.'Properties'.'Name' | - Sort-Object -Property @{ - 'Expression' = { - [uint16]($Script:WantedOrder.IndexOf($_)) - } - } | ForEach-Object -Process { + # Add properties from $Current to $Sorted ordered by $WantedOrder + $indexMap = @{} + for ($i = 0; $i -lt $Script:WantedOrder.Count; $i++) { $indexMap[$Script:WantedOrder[$i]] = [uint16]$i } + $JsonAsObject.PSObject.Properties.Name | + Sort-Object -Property @{ Expression = { $indexMap[$_] } } | + ForEach-Object -Process {
312-319: Also handle arrays of PSCustomObject when sorting children.Some manifest children may be arrays of objects; process them too.
- # Order childs alphabetically recursively, if parent key is of type PSCustomObject - foreach ( - $Key in $SortedObject.'PSObject'.'Properties'.Where{ - $_.'TypeNameOfValue' -eq 'System.Management.Automation.PSCustomObject' - }.'Name' - ) { - $SortedObject.$Key = [PSCustomObject](Sort-PSCustomObjectKeysRecursively -InputObject $SortedObject.$Key) - } + # Order child objects alphabetically (recursively). Also handle arrays of PSCustomObject. + foreach ($prop in $SortedObject.PSObject.Properties) { + $val = $prop.Value + if ($val -is [System.Management.Automation.PSCustomObject]) { + $SortedObject.$($prop.Name) = Sort-PSCustomObjectKeysRecursively -InputObject $val + } elseif ($val -is [System.Array] -and $val.Count -gt 0 -and $val[0] -is [System.Management.Automation.PSCustomObject]) { + $SortedObject.$($prop.Name) = @($val | ForEach-Object { Sort-PSCustomObjectKeysRecursively -InputObject $_ }) + } + }
246-260: Prefer [pscustomobject]@{} over [PSCustomObject]::new(); add array handling in recursion.Keeps PS 5.1 friendly and handles nested arrays-of-objects.
Please confirm on Windows PowerShell 5.1 that [PSCustomObject]::new() is available in your environment; if not, use the patch below.
- # Recurse if node value is of type [PSCustomObject] - if ($Node -is [System.Management.Automation.PSCustomObject]) { - $OrderedPSCustomObject = [PSCustomObject]::new() - $SortedProperties = $Node.PSObject.Properties | Sort-Object -Property Name - foreach ($Property in $SortedProperties) { - $null = Add-Member -InputObject $OrderedPSCustomObject -NotePropertyName $Property.Name -NotePropertyValue ( - Convert-Node -Node $Property.Value - ) - } - return $OrderedPSCustomObject - } - # Else return the value as is - else { - return $Node - } + switch ($true) { + { $Node -is [System.Management.Automation.PSCustomObject] } { + $ordered = [pscustomobject]@{} + $Node.PSObject.Properties | Sort-Object -Property Name | ForEach-Object { + $null = Add-Member -InputObject $ordered -NotePropertyName $_.Name -NotePropertyValue (Convert-Node -Node $_.Value) + } + return $ordered + } + { $Node -is [System.Array] } { + return @($Node | ForEach-Object { Convert-Node -Node $_ }) + } + default { return $Node } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/manifest.ps1(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-22T14:53:13.397Z
Learnt from: o-l-a-v
PR: ScoopInstaller/Scoop#6494
File: lib/manifest.ps1:281-289
Timestamp: 2025-09-22T14:53:13.397Z
Learning: In PowerShell, string[] arrays can use the .IndexOf() instance method to find the index of an element, as demonstrated by user o-l-a-v with working example: $([string[]]('0','1','2')).IndexOf('1') returns 1.
Applied to files:
lib/manifest.ps1
📚 Learning: 2025-09-22T14:53:13.397Z
Learnt from: o-l-a-v
PR: ScoopInstaller/Scoop#6494
File: lib/manifest.ps1:281-289
Timestamp: 2025-09-22T14:53:13.397Z
Learning: PowerShell arrays have IndexOf as an instance method available directly on array objects, so `$array.IndexOf($item)` is valid syntax and does not require using the static [Array]::IndexOf() method.
Applied to files:
lib/manifest.ps1
🔇 Additional comments (1)
lib/manifest.ps1 (1)
281-289: Guard schema loading and resolve the schema path explicitly.If schema.json can’t be parsed, later member access can null‑ref or misbehave. Abort early with a clear message.
- if ([string]::IsNullOrWhiteSpace($Script:WantedOrder)) { - $Script:WantedOrder = [string[]]( - ( - parse_json -path ( - '{0}\..\schema.json' -f $PSScriptRoot - ) - ).'properties'.'PSObject'.'Properties'.'Name' - ) - } + if (-not $Script:WantedOrder) { + $schemaPath = Join-Path -Path $PSScriptRoot -ChildPath '..\schema.json' + $schema = parse_json -path $schemaPath + if ($null -eq $schema -or $null -eq $schema.properties) { + abort ("Unable to load 'properties' from schema.json at '{0}'." -f $schemaPath) + } + $Script:WantedOrder = @($schema.properties.PSObject.Properties.Name) + }
As I mentioned earlier, this is the code implementation of my previous idea. Can this help us go further in a simpler way? Just a thought. function Get-RootPropertiesObject {
[OutputType([PSCustomObject])]
param()
if (-not $Script:RootPropertiesObject) {
$Script:RootPropertiesObject = [PSCustomObject](
(
parse_json -path (
'{0}\..\schema.json' -f $PSScriptRoot
)
).'properties'
)
}
return $Script:RootPropertiesObject
}
function Sort-Properties {
param(
[Parameter(Mandatory)]
$Properties,
[Parameter(Mandatory = $false)]
[PSCustomObject] $PropertiesObjectDefault = (Get-RootPropertiesObject),
[Parameter(Mandatory = $false)]
[PSCustomObject] $PropertiesObjectOverride = $null
)
if ($Properties -is [PSCustomObject]) {
$OrderDefault = $PropertiesObjectDefault.'PSObject'.'Properties'.'Name'
$OrderOverride = $PropertiesObjectOverride.'PSObject'.'Properties'.'Name'
[PSCustomObject] $PropertiesObject = if ($PropertiesObjectOverride) {
$PropertiesObjectOverride
} else {
$PropertiesObjectDefault
}
$PropertyNames = $Properties.'PSObject'.'Properties'.'Name'
$SortedProperties = [PSCustomObject]::new()
$SortedPropertyNames = @($PropertyNames.Where{ $_ -in $OrderOverride } | Sort-Object { $OrderOverride.IndexOf($_) }) +
@($PropertyNames.Where{ ($_ -notin $OrderOverride) -and ($_ -in $OrderDefault)} | Sort-Object { $OrderDefault.IndexOf($_) }) +
@($PropertyNames.Where{ ($_ -notin $OrderOverride) -and ($_ -notin $OrderDefault)})
$SortedPropertyNames | ForEach-Object {
$SortedProperty = Sort-Properties -Properties $Properties.$_ -PropertiesObjectDefault $PropertiesObjectDefault -PropertiesObjectOverride $PropertiesObject.$_.'properties'
if ($SortedProperty) {
Add-Member -InputObject $SortedProperties -NotePropertyName $_ -NotePropertyValue $SortedProperty | Out-Null
}
}
if ((-not $SortedProperties.PSObject.Properties) -or ($SortedProperties.PSObject.Properties.Count -eq 0)) {
$SortedProperties = $null
}
return $SortedProperties
}
if ($Properties -is [array]) {
$SortedProperties = @()
for($i = 0; $i -lt $Properties.Count; ++$i) {
$SortedProperties += @(Sort-Properties -Properties $Properties[$i] -PropertiesObjectDefault $PropertiesObjectDefault)
}
return ,$SortedProperties
}
return $Properties
}
function Get-SortedManifest {
[OutputType([PSCustomObject])]
param(
[Parameter(Mandatory)]
[ValidateScript({ $null -ne $_ -and $_ -ne [PSCustomObject]::new() })]
[PSCustomObject] $Manifest
)
Sort-Properties -Properties $Manifest -PropertiesObjectDefault (Get-RootPropertiesObject)
} |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/Scoop-Manifest.Tests.ps1 (1)
14-16: Assert return semantics and cover whitespace variantsGood to guard against exceptions. Strengthen by asserting the expected return (e.g., $null) and include whitespace-only inputs to prevent regressions across PS 5.1/7.x.
- It 'should not throw if provided an empty string' { - { parse_json '' } | Should -Not -Throw - } + It 'handles empty/whitespace input without throwing and returns $null: <_>' -ForEach @('', ' ', "`r`n") { + { parse_json $_ } | Should -Not -Throw + (parse_json $_) | Should -Be $null + }If $null isn’t the intended contract, replace the assertion with the expected value (e.g., an empty hashtable or PSCustomObject) and document it in the function help.
|
I don't see how that is simpler. Also the first function won't, as PSCustomObject.properties won't output anything. Is it LLM generated? |
Possibly more comprehensive.
It's actually 'properties' in schema.json.
Yes, after editing and modifying it, it runs fine on my local machine. |
That's subjective. I think not using aliases, using explicit types, and using named parameters with functions are more readable, maintainable. But I'm down with whatever Scoop maintainers want, as long as we can get the functionality we want here. 😊
Isn't it
Ok. |
Description
schema.jsonso it matches the wanted order.manifest.ps1Sort-ScoopManifestProperties:schema.jsonproperties.PSCustomObject.formatjson.ps1.Sort-PSCustomObjectKeysRecursively:Sort-ScoopManifestPropertiesfor manifest 1 level child properties that are of type[PSCustomObject].formatjson.ps1use new functionSort-ScoopManifestProperties.Motivation and Context
Closes #6491.
How Has This Been Tested?
Checklist:
developbranch.formatjson.ps1was added with feat: Add lint checks GithubActions#61: https://github.com/ScoopInstaller/GithubActions/wiki/Pull-Request-ChecksSummary by CodeRabbit
New Features
Refactor
Tests
Documentation