Rework type setting and default configuration loading#4
Merged
Conversation
Signed-off-by: WithLithum <WithLithum@outlook.com>
Signed-off-by: WithLithum <WithLithum@outlook.com>
This commit reworked the type settings function by turning dictionaries for return and parameter types into 6 common properties which all other parameter types are inferred from. This significantly reduced the burden for writing configuration files. BREAKING CHANGE: All existing generator settings will have to adapt to the changes of this commit. Signed-off-by: WithLithum <WithLithum@outlook.com>
Signed-off-by: WithLithum <WithLithum@outlook.com>
This commit makes default settings hardcoded rather than bundled within the Data directory. Signed-off-by: WithLithum <WithLithum@outlook.com>
Signed-off-by: WithLithum <WithLithum@outlook.com>
Signed-off-by: WithLithum <WithLithum@outlook.com>
Signed-off-by: WithLithum <WithLithum@outlook.com>
Signed-off-by: WithLithum <WithLithum@outlook.com>
Signed-off-by: WithLithum <WithLithum@outlook.com>
📝 WalkthroughWalkthroughConsolidates type mapping into a new GeneratorTypeSettings contract, adds generator Ids and DefaultSettings, updates generators and consumers to use the centralized settings, replaces JSON defaults with in-code defaults, adds tests, and updates tooling/docs. ChangesType Settings Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.zed/tasks.json (1)
10-10: ⚡ Quick winRemove trailing comma for strict JSON compliance.
Standard JSON (RFC 8259) does not allow trailing commas after the last element in an array or object. While some parsers are lenient, this may cause issues with strict JSON validators.
🔧 Proposed fix
"command": "dotnet run --project Benchmarks --configuration Release", - }, + } ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.zed/tasks.json at line 10, Remove the trailing comma after the last element in the tasks array so the JSON ends with the closing bracket ']' (not ',]'). Locate the tasks array's closing ']' and delete the comma character immediately before it, then re-validate the JSON to ensure no other trailing commas remain.WithLithum.NativeWrapperGen/Program.cs (1)
74-74: ⚡ Quick winReplace duplicated default generator literal with
VDotNetGenerator.Id.Line 74 and Line 118 hardcode
"shvdn"even though an ID constant exists; this can silently drift if the ID is changed in one place.♻️ Proposed fix
- DefaultValueFactory = _ => "shvdn", + DefaultValueFactory = _ => VDotNetGenerator.Id, @@ - var generator = result.GetValue(GeneratorOption) ?? "shvdn"; + var generator = result.GetValue(GeneratorOption) ?? VDotNetGenerator.Id;Also applies to: 118-118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WithLithum.NativeWrapperGen/Program.cs` at line 74, Two places hardcode the generator ID string "shvdn" in the DefaultValueFactory lambdas; replace those literals with the canonical constant VDotNetGenerator.Id so the value stays single-sourced. Locate the DefaultValueFactory assignments (the lambda returning "shvdn") and change them to return VDotNetGenerator.Id instead, and ensure the file has the correct using or fully-qualified reference to VDotNetGenerator so the symbol resolves.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.editorconfig:
- Line 6: Fix the typo in the editorconfig header template: update the value of
the file_header_template key so the SPDX identifier is correct by replacing
"SDPX-FileCopyrightText" with "SPDX-FileCopyrightText" (preserve the rest of the
string including "2025-2026 WithLithum\nSPDX-License-Identifier: Apache-2.0").
In `@NativeWrapperGenTests/SettingTests.cs`:
- Around line 53-64: The test GetParamTypeName_AnyPointerType_ReturnsHandleType
is using the wrong enum value (SCPT.Entity) so it never validates the AnyPointer
mapping; update the Arrange input to use the AnyPointer enum member (e.g.,
replace SCPT.Entity with SCPT.AnyPointer or the correct SCPT.AnyPointerType
constant) so settings.GetTypeName(input) exercises the AnyPointerType code path
while keeping Base with { HandleType = "PoolHandle" } and the existing Assert.
In `@WithLithum.NativeWrapperGen/Models/Settings/GeneratorTypeSettings.cs`:
- Around line 1-2: Replace the misspelled SPDX token in the file header: change
the string "SDPX-FileCopyrightText" to the correct "SPDX-FileCopyrightText" at
the top of WithLithum.NativeWrapperGen/Models/Settings/GeneratorTypeSettings.cs
so license tooling recognizes the header.
---
Nitpick comments:
In @.zed/tasks.json:
- Line 10: Remove the trailing comma after the last element in the tasks array
so the JSON ends with the closing bracket ']' (not ',]'). Locate the tasks
array's closing ']' and delete the comma character immediately before it, then
re-validate the JSON to ensure no other trailing commas remain.
In `@WithLithum.NativeWrapperGen/Program.cs`:
- Line 74: Two places hardcode the generator ID string "shvdn" in the
DefaultValueFactory lambdas; replace those literals with the canonical constant
VDotNetGenerator.Id so the value stays single-sourced. Locate the
DefaultValueFactory assignments (the lambda returning "shvdn") and change them
to return VDotNetGenerator.Id instead, and ensure the file has the correct using
or fully-qualified reference to VDotNetGenerator so the symbol resolves.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbdfcda1-76a7-484a-b1d7-a975367224de
📒 Files selected for processing (21)
.editorconfig.zed/tasks.jsonAGENTS.mdBenchmarks/GenerateToNull.csBenchmarks/Program.csGenerateNativeWrapper.ps1NativeWrapperGenTests/SettingTests.csWithLithum.NativeWrapperGen/ConfigFileHelper.csWithLithum.NativeWrapperGen/Data/RPHSettings.jsonWithLithum.NativeWrapperGen/Data/SHVDNSettings.jsonWithLithum.NativeWrapperGen/Generation/DocGenerator.csWithLithum.NativeWrapperGen/Generation/Hooks/CSharpGenerator.csWithLithum.NativeWrapperGen/Generation/Hooks/RageGenerator.csWithLithum.NativeWrapperGen/Generation/Hooks/VDotNetGenerator.csWithLithum.NativeWrapperGen/Generation/ParamUtil.csWithLithum.NativeWrapperGen/Generation/WrapperFileGenerator.Total.csWithLithum.NativeWrapperGen/Models/GeneratorSettings.csWithLithum.NativeWrapperGen/Models/Settings/GeneratorTypeSettings.csWithLithum.NativeWrapperGen/Program.csWithLithum.NativeWrapperGen/Serialization/ScriptCommandInfoContext.csdocs/CODESTYLES.md
💤 Files with no reviewable changes (2)
- WithLithum.NativeWrapperGen/Data/SHVDNSettings.json
- WithLithum.NativeWrapperGen/Data/RPHSettings.json
Signed-off-by: WithLithum <WithLithum@outlook.com>
Signed-off-by: WithLithum <WithLithum@outlook.com>
Signed-off-by: WithLithum <WithLithum@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
WithLithum.NativeWrapperGen/Generation/Hooks/RageGenerator.cs (1)
11-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid exposing mutable global defaults via
public static readonly.Line 11 publishes a mutable
GeneratorSettingsinstance globally. Any caller can mutate it, causing nondeterministic behavior across generators/tests. Prefer an immutable template or a factory that returns a fresh instance.Suggested fix
- public static readonly GeneratorSettings DefaultSettings = new() - { - TypeSettings = new() - { - AnyParameterType = "global::Rage.Native.NativeArgument", - AnyPointerType = "global::System.IntPtr", - AnyReturnType = "int", - HandleType = "uint", - HashType = "uint", - Vector3Type = "global::Rage.Vector3", - PlayerIdType = "int" - }, - Accessibility = "public" - }; + public static GeneratorSettings CreateDefaultSettings() => new() + { + TypeSettings = new() + { + AnyParameterType = "global::Rage.Native.NativeArgument", + AnyPointerType = "global::System.IntPtr", + AnyReturnType = "int", + HandleType = "uint", + HashType = "uint", + Vector3Type = "global::Rage.Vector3", + PlayerIdType = "int" + }, + Accessibility = "public" + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WithLithum.NativeWrapperGen/Generation/Hooks/RageGenerator.cs` around lines 11 - 24, The public static readonly GeneratorSettings DefaultSettings exposes a mutable global template (including its nested TypeSettings) and should be replaced with a factory or immutable template to avoid shared mutations; change DefaultSettings to a method or property that returns a new GeneratorSettings instance (or deep-cloned instance) each call (e.g., CreateDefaultSettings or DefaultSettingsFactory) and ensure the nested TypeSettings object is freshly created with the same fields (AnyParameterType, AnyPointerType, AnyReturnType, HandleType, HashType, Vector3Type, PlayerIdType) and Accessibility set to "public" so callers receive an independent mutable copy instead of a shared global.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@WithLithum.NativeWrapperGen/Generation/Hooks/RageGenerator.cs`:
- Around line 11-24: The public static readonly GeneratorSettings
DefaultSettings exposes a mutable global template (including its nested
TypeSettings) and should be replaced with a factory or immutable template to
avoid shared mutations; change DefaultSettings to a method or property that
returns a new GeneratorSettings instance (or deep-cloned instance) each call
(e.g., CreateDefaultSettings or DefaultSettingsFactory) and ensure the nested
TypeSettings object is freshly created with the same fields (AnyParameterType,
AnyPointerType, AnyReturnType, HandleType, HashType, Vector3Type, PlayerIdType)
and Accessibility set to "public" so callers receive an independent mutable copy
instead of a shared global.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2c7fe84-b780-4ed8-8fe7-c7566de04c77
📒 Files selected for processing (11)
.editorconfig.zed/tasks.jsonNativeWrapperGenTests/SettingTests.csWithLithum.NativeWrapperGen/ConfigFileHelper.csWithLithum.NativeWrapperGen/Generation/Hooks/CSharpGenerator.csWithLithum.NativeWrapperGen/Generation/Hooks/RageGenerator.csWithLithum.NativeWrapperGen/Generation/Hooks/VDotNetGenerator.csWithLithum.NativeWrapperGen/Generation/IShimGenerator.csWithLithum.NativeWrapperGen/Models/GeneratorSettings.csWithLithum.NativeWrapperGen/Models/Settings/GeneratorTypeSettings.csWithLithum.NativeWrapperGen/Program.cs
✅ Files skipped from review due to trivial changes (3)
- WithLithum.NativeWrapperGen/Generation/IShimGenerator.cs
- .zed/tasks.json
- .editorconfig
🚧 Files skipped from review as they are similar to previous changes (6)
- WithLithum.NativeWrapperGen/Generation/Hooks/VDotNetGenerator.cs
- WithLithum.NativeWrapperGen/ConfigFileHelper.cs
- WithLithum.NativeWrapperGen/Models/GeneratorSettings.cs
- WithLithum.NativeWrapperGen/Program.cs
- WithLithum.NativeWrapperGen/Models/Settings/GeneratorTypeSettings.cs
- WithLithum.NativeWrapperGen/Generation/Hooks/CSharpGenerator.cs
Owner
Author
|
@coderabbitai - Regarding Every property of Unless you are trying to say that callers would mess with |
|
✏️ Learnings added
✅ Actions performedComments resolved. Approval is disabled; enable |
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.
This pull request reworked how script command types are mapped to CLR types in the configuration file.
A new type setting system is introduced, which uses 7 properties to group parameter types and return types of the same kind, and map them to their corresponding CLR types, dramatically reducing the chance of human error when editing configuration files.
AnyparametersAnyParameterTypeAnypointersAnyPointerTypeAnyreturn valueAnyReturnTypeHandleTypeHashTypeVector3TypePlayerIdTypeTwo dictionary properties to override the default mapping logic are also provided.
The change also allowed default configuration files to be easily hardcoded into a
readonlyfield, eliminating the two default configuration files inDatadirectory.Notes
e8aa4d4andba72cc6) already exists intrunk(as3f6f4baand754be4f) because they were retroactively added totrunkand is not part of the changes made by this Pull Request. The actual changes start at567c185.Summary by CodeRabbit
Tests
Documentation
Chores