Merged
Conversation
Reviewer's GuideUpdates the docs metadata sync script to derive the Terraria version from restored NuGet assets (OTAPI.dll) instead of a hard-coded helper, wires this into the CI build so restore happens before the check, and refreshes the README version matrix values and wording accordingly. Sequence diagram for CI docs metadata sync using restored NuGet assetssequenceDiagram
actor Developer
participant GitHubActions
participant DotNetCLI
participant SyncDocMetadataScript
participant RestoreAssetsJson as project.assets.json
participant OTAPIAssembly as OTAPI.dll
Developer->>GitHubActions: Push commit / open PR
GitHubActions->>GitHubActions: Checkout repository
GitHubActions->>GitHubActions: Setup .NET SDK
GitHubActions->>DotNetCLI: dotnet restore PROJECT_PATH
DotNetCLI->>RestoreAssetsJson: Generate restore assets
GitHubActions->>SyncDocMetadataScript: Run sync-doc-metadata.ps1 -Mode check
SyncDocMetadataScript->>RestoreAssetsJson: Get-RestoreAssets(Path, ProjectPath)
RestoreAssetsJson-->>SyncDocMetadataScript: Parsed assets object
SyncDocMetadataScript->>SyncDocMetadataScript: Get-TerrariaVersionFromRestoreAssets(Assets, TargetFramework, ProjectPath)
SyncDocMetadataScript->>SyncDocMetadataScript: Resolve-RestoredPackageAssetPath(Assets, TargetFramework, PackageId, AssetFileName, ProjectPath)
SyncDocMetadataScript->>OTAPIAssembly: Get-AssemblyFileVersion(Path)
OTAPIAssembly-->>SyncDocMetadataScript: FileVersion (TerrariaVersion)
SyncDocMetadataScript->>SyncDocMetadataScript: Build-EnglishVersionBlock(Metadata)
SyncDocMetadataScript->>SyncDocMetadataScript: Build-ChineseVersionBlock(Metadata)
SyncDocMetadataScript->>SyncDocMetadataScript: Sync-MarkedBlock on README files
alt README out of sync and Mode is check
SyncDocMetadataScript-->>GitHubActions: Throw error (metadata out of sync)
GitHubActions-->>Developer: CI job fails, instruct to run script in apply mode
else README in sync
SyncDocMetadataScript-->>GitHubActions: Success message
GitHubActions-->>Developer: CI job passes
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
Resolve-RestoredPackageAssetPath, you assumeruntimeassets always exist for the target; consider explicitly handling the case whereruntimeis null/missing (e.g., falling back tocompileorruntimeTargets) to avoid unexpected null dereferences on different package layouts. - The
Get-RestoredPackageLibraryfunction picks the first library whose name matches the^{PackageId}/pattern; if multiple versions of the same package are present, it may be worth tightening the match (e.g., including version or target) or at least asserting that only one candidate exists to make behavior predictable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Resolve-RestoredPackageAssetPath`, you assume `runtime` assets always exist for the target; consider explicitly handling the case where `runtime` is null/missing (e.g., falling back to `compile` or `runtimeTargets`) to avoid unexpected null dereferences on different package layouts.
- The `Get-RestoredPackageLibrary` function picks the first library whose name matches the `^{PackageId}/` pattern; if multiple versions of the same package are present, it may be worth tightening the match (e.g., including version or target) or at least asserting that only one candidate exists to make behavior predictable.
## Individual Comments
### Comment 1
<location path="tools/sync-doc-metadata.ps1" line_range="62" />
<code_context>
+ throw "Could not locate restore assets '$Path'. Run: dotnet restore '$ProjectPath'"
+ }
+
+ return (Get-Content -Path $Path -Raw -Encoding UTF8 | ConvertFrom-Json)
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** project.assets.json is deeply nested and may be truncated without an explicit ConvertFrom-Json -Depth
On Windows PowerShell, `ConvertFrom-Json` defaults to `-Depth 2`, which will silently truncate this deeply nested file and can lead to subtle runtime issues when accessing nested properties. Please specify an explicit depth (e.g. `ConvertFrom-Json -Depth 100`) so the assets file is fully parsed across PowerShell versions.
</issue_to_address>
### Comment 2
<location path="tools/sync-doc-metadata.ps1" line_range="130-132" />
<code_context>
+
+ $library = Get-RestoredPackageLibrary -Assets $Assets -PackageId $PackageId
+ $targetPackage = Get-RestoredTargetPackageEntry -Assets $Assets -TargetFramework $TargetFramework -PackageLibraryName $library.Name
+ $runtimeAssets = @($targetPackage.runtime.PSObject.Properties)
+ $assetRelativePath = $runtimeAssets |
+ Where-Object { [System.IO.Path]::GetFileName([string]$_.Name) -ieq $AssetFileName } |
</code_context>
<issue_to_address>
**suggestion:** Handle missing runtime assets more gracefully to avoid null-reference errors
If the resolved package entry has no `runtime` section (or it’s empty), `$targetPackage.runtime` will be `$null` and this line will throw a generic null-reference error. Please add an explicit `$null` check before accessing `.runtime` and raise a clearer, targeted error when no runtime assets exist for the resolved package/TFM, consistent with the other guarded cases in this script.
```suggestion
$library = Get-RestoredPackageLibrary -Assets $Assets -PackageId $PackageId
$targetPackage = Get-RestoredTargetPackageEntry -Assets $Assets -TargetFramework $TargetFramework -PackageLibraryName $library.Name
if (-not $targetPackage.runtime) {
throw "Restored package '$PackageId' for target framework '$TargetFramework' does not contain any runtime assets."
}
$runtimeAssets = @($targetPackage.runtime.PSObject.Properties)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| throw "Could not locate restore assets '$Path'. Run: dotnet restore '$ProjectPath'" | ||
| } | ||
|
|
||
| return (Get-Content -Path $Path -Raw -Encoding UTF8 | ConvertFrom-Json) |
There was a problem hiding this comment.
issue (bug_risk): project.assets.json is deeply nested and may be truncated without an explicit ConvertFrom-Json -Depth
On Windows PowerShell, ConvertFrom-Json defaults to -Depth 2, which will silently truncate this deeply nested file and can lead to subtle runtime issues when accessing nested properties. Please specify an explicit depth (e.g. ConvertFrom-Json -Depth 100) so the assets file is fully parsed across PowerShell versions.
Comment on lines
+130
to
+132
| $library = Get-RestoredPackageLibrary -Assets $Assets -PackageId $PackageId | ||
| $targetPackage = Get-RestoredTargetPackageEntry -Assets $Assets -TargetFramework $TargetFramework -PackageLibraryName $library.Name | ||
| $runtimeAssets = @($targetPackage.runtime.PSObject.Properties) |
There was a problem hiding this comment.
suggestion: Handle missing runtime assets more gracefully to avoid null-reference errors
If the resolved package entry has no runtime section (or it’s empty), $targetPackage.runtime will be $null and this line will throw a generic null-reference error. Please add an explicit $null check before accessing .runtime and raise a clearer, targeted error when no runtime assets exist for the resolved package/TFM, consistent with the other guarded cases in this script.
Suggested change
| $library = Get-RestoredPackageLibrary -Assets $Assets -PackageId $PackageId | |
| $targetPackage = Get-RestoredTargetPackageEntry -Assets $Assets -TargetFramework $TargetFramework -PackageLibraryName $library.Name | |
| $runtimeAssets = @($targetPackage.runtime.PSObject.Properties) | |
| $library = Get-RestoredPackageLibrary -Assets $Assets -PackageId $PackageId | |
| $targetPackage = Get-RestoredTargetPackageEntry -Assets $Assets -TargetFramework $TargetFramework -PackageLibraryName $library.Name | |
| if (-not $targetPackage.runtime) { | |
| throw "Restored package '$PackageId' for target framework '$TargetFramework' does not contain any runtime assets." | |
| } | |
| $runtimeAssets = @($targetPackage.runtime.PSObject.Properties) |
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
1 participant
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.
Summary by Sourcery
Update documentation metadata generation to derive the referenced Terraria version from restored package assets and align CI to restore dependencies before running the sync check.
Enhancements:
CI:
Documentation: