improve lua error handling - #16
Conversation
All native zig functions are wrapped by a lua helper. Avoids calling assert in manifests and avoids calling lua_Lerror which makes defers not work.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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: 2
🤖 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 `@src/actions/build_package.zig`:
- Line 390: Update the error message in the Lua argument-validation logic to use
the correctly spaced phrase “at least” instead of “atleast,” preserving the rest
of the message unchanged.
In `@src/error_wrapper.lua`:
- Around line 5-9: Update the outer wrapper setup in the returned function
around native so it captures the current global error function in a local before
creating and returning the inner wrapper. Use that captured function in the
failure branch of the inner wrapper instead of resolving global error at call
time, while preserving the existing level argument and return behavior.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 209690a2-cedc-45e1-a824-75407a933356
📒 Files selected for processing (5)
src/actions/build_package.zigsrc/actions/info.zigsrc/actions/install.zigsrc/error_wrapper.luasrc/lua_helpers.zig
| if (n_args < 1) { | ||
| lua.pushNil(); | ||
| _ = lua.pushLString("run requires atleast 1 arg"); | ||
| _ = lua.pushLString("run requires atleast 1 argument"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the error text.
Change "atleast" to "at least".
🤖 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 `@src/actions/build_package.zig` at line 390, Update the error message in the
Lua argument-validation logic to use the correctly spaced phrase “at least”
instead of “atleast,” preserving the rest of the message unchanged.
| return function(native) | ||
| return function(...) | ||
| local value, failure = native(...) | ||
| if failure ~= nil then | ||
| error(failure, 2) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Capture the Lua error function during wrapper setup.
A manifest can replace global error before a checked native function fails. The wrapper can then return normally after a failed native call. Capture error in a local before returning the wrapper.
Proposed fix
+local lua_error = error
+
return function(native)
return function(...)
local value, failure = native(...)
if failure ~= nil then
- error(failure, 2)
+ lua_error(failure, 2)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return function(native) | |
| return function(...) | |
| local value, failure = native(...) | |
| if failure ~= nil then | |
| error(failure, 2) | |
| local lua_error = error | |
| return function(native) | |
| return function(...) | |
| local value, failure = native(...) | |
| if failure ~= nil then | |
| lua_error(failure, 2) |
🤖 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 `@src/error_wrapper.lua` around lines 5 - 9, Update the outer wrapper setup in
the returned function around native so it captures the current global error
function in a local before creating and returning the inner wrapper. Use that
captured function in the failure branch of the inner wrapper instead of
resolving global error at call time, while preserving the existing level
argument and return behavior.
1 participant
All native zig functions are wrapped by a lua helper.
Avoids calling assert in manifests and avoids calling lua_Lerror which
makes defers not work.
Summary by CodeRabbit
Bug Fixes
nilresults.Reliability