refactor: adopt lua-resty-etcd code style conventions#2
Merged
Conversation
- Align imports with = signs, localize all builtins at file top - Use prefixed aliases (tab_insert, str_find, sub_str, etc.) - Replace OOP string method calls with localized functions - Remove LDoc annotations, use simple -- comments - Rename metatables (_VMT -> _validator_mt, _MT -> _router_mt) - Use inline version field (version = 0.1) Infrastructure: - Add .luacheckrc (std=ngx_lua, ignore 542) - Rewrite Makefile with self-documenting ### target: pattern - Add install, dev, help targets - Add master rockspec Documentation: - Rewrite README with rst-style headers, TOC, badges - Create separate api.md with detailed API documentation - Fix README to match actual code (remove undocumented options) - Add readOnly/writeOnly and form-urlencoded to feature matrix
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds lint/config and packaging files, expands Makefile targets, adds API docs and README rework, applies local aliasing and metatable renames across core modules, updates tests to use renamed skip option Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 (3)
Makefile (1)
10-29: Add anallphony alias to satisfy tooling and conventions.This removes
checkmakenoise and gives a predictable default target.♻️ Proposed update
-.PHONY: test test-unit test-conformance benchmark lint dev install clean help +.PHONY: all test test-unit test-conformance benchmark lint dev install clean help + +### all: Default target +all: test ### help: Show Makefile rules help:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 10 - 29, Add a conventional phony "all" target that invokes the default build/test (make all -> make test) and include it in the .PHONY list; specifically update the .PHONY line to include "all" and add an "all:" target that depends on "test" (so the existing "test" target runs when users or tooling invoke the default target).api.md (1)
13-33: Keep compile argument naming consistent in docs.The syntax uses
spec_strwhile the example usesspec_json. Using one name throughout improves scanability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api.md` around lines 13 - 33, The docs use two different parameter names for ov.compile's first argument (`spec_str` in the signature and `spec_json` in the example); update the example to use the same name as the signature (use `spec_str` everywhere) so ov.compile, the parameter description, and the example consistently reference `spec_str` for the raw OpenAPI JSON input.lib/resty/openapi_validator/params.lua (1)
158-170: Makesplitdelimiter-length aware.
from = pos + 1only works for single-character delimiters. Advancing by#delimmakes the helper correct for all delimiters.♻️ Proposed refactor
local function split(s, delim) local result = {} local from = 1 local pos while true do pos = str_find(s, delim, from, true) if not pos then tab_insert(result, sub_str(s, from)) break end tab_insert(result, sub_str(s, from, pos - 1)) - from = pos + 1 + from = pos + `#delim` end return result end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resty/openapi_validator/params.lua` around lines 158 - 170, The split function advances the search pointer by 1 which only works for single-character delimiters; update the loop in function split to advance from by the delimiter length instead of 1 (i.e., set from = pos + `#delim` or equivalent) so multi-character delimiters are handled correctly—adjust references around str_find, delim, pos and from accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/resty/openapi_validator/router.lua`:
- Around line 88-99: The content-type matching loop in router.lua can miss
case-variant keys (e.g., "Application/JSON") and then falls back to unordered
pairs; normalize header keys to lowercase before matching: in the loop that
iterates over content (variables ct, media) compute a lowercase version of ct
(using string.lower) and test if ct_lower == "*/*" or string.find(ct_lower,
"json") (instead of using str_find on the original ct); set primary_schema from
media.schema when matched and retain the existing fallback loop unchanged.
In `@README.md`:
- Around line 47-50: The shell example in README.md uses leading "$ " prompts
which triggers markdownlint MD014; edit the fenced code block containing the
commands (the lines showing git clone, cd, make dev, sudo make install) to
remove the "$ " prefixes so the block contains plain commands (git clone ..., cd
..., make dev, sudo make install) without shell prompts, ensuring the example
passes MD014.
In `@rockspec/lua-resty-openapi-validator-master-0.1-0.rockspec`:
- Around line 16-20: The rockspec is missing the lua-cjson dependency which
causes runtime failures because lib/resty/openapi_validator/loader.lua and
lib/resty/openapi_validator/body.lua unconditionally require "cjson.safe";
update the dependencies block in the rockspec to include "lua-cjson" (add
"lua-cjson" to the dependencies table) so the package manager installs cjson, or
alternatively modify the require calls in loader.lua and body.lua to use pcall
like lib/resty/openapi_validator/params.lua does (wrap require("cjson.safe") in
pcall and handle absence gracefully).
---
Nitpick comments:
In `@api.md`:
- Around line 13-33: The docs use two different parameter names for ov.compile's
first argument (`spec_str` in the signature and `spec_json` in the example);
update the example to use the same name as the signature (use `spec_str`
everywhere) so ov.compile, the parameter description, and the example
consistently reference `spec_str` for the raw OpenAPI JSON input.
In `@lib/resty/openapi_validator/params.lua`:
- Around line 158-170: The split function advances the search pointer by 1 which
only works for single-character delimiters; update the loop in function split to
advance from by the delimiter length instead of 1 (i.e., set from = pos + `#delim`
or equivalent) so multi-character delimiters are handled correctly—adjust
references around str_find, delim, pos and from accordingly.
In `@Makefile`:
- Around line 10-29: Add a conventional phony "all" target that invokes the
default build/test (make all -> make test) and include it in the .PHONY list;
specifically update the .PHONY line to include "all" and add an "all:" target
that depends on "test" (so the existing "test" target runs when users or tooling
invoke the default target).
🪄 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: 2c5e845d-6632-404e-8e1f-0f1f97658140
📒 Files selected for processing (13)
.luacheckrcMakefileREADME.mdapi.mdlib/resty/openapi_validator/body.lualib/resty/openapi_validator/errors.lualib/resty/openapi_validator/init.lualib/resty/openapi_validator/loader.lualib/resty/openapi_validator/normalize.lualib/resty/openapi_validator/params.lualib/resty/openapi_validator/refs.lualib/resty/openapi_validator/router.luarockspec/lua-resty-openapi-validator-master-0.1-0.rockspec
- router: use case-insensitive content-type matching for JSON detection - readme: remove $ prompts from shell examples (MD014) - rockspec: add lua-cjson to dependencies in both rockspecs
nic-6443
reviewed
Apr 21, 2026
nic-6443
reviewed
Apr 21, 2026
- Add luacheck lint step to test.yml CI workflow - Rename skip.readOnly -> skip.read_only, skip.writeOnly -> skip.write_only to follow Lua snake_case convention - Update api.md and tests accordingly
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
45-48: Pinluacheckversion to ensure CI reproducibility.At line 47, installing the latest
luacheckwithout pinning can introduce nondeterministic CI behavior—each run may fetch a different version if upstream releases occur between runs. Upstream changes to lint rules or strictness could cause unexpected failures. Pin to a known-good version (e.g.,1.2.0-1, the latest stable release on LuaRocks) usingsudo luarocks install luacheck 1.2.0-1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 45 - 48, The CI step named "Lint" installs luacheck without a version pin which can make builds non-reproducible; update the workflow to pin luacheck to a stable version (e.g., "1.2.0-1") by changing the install command in the Lint job to install that specific version so all CI runs use the same luacheck release.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@t/conformance/test_validate_readonly.lua`:
- Around line 61-63: Update the test description string in the T.describe(...)
call so it matches the renamed skip key used in the test body: change the phrase
"skip.readOnly" to "skip.read_only" (the test uses read_only when calling
v:validate_request and make_spec("readOnly")), ensuring the human-readable
description aligns with the actual skip key name.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 45-48: The CI step named "Lint" installs luacheck without a
version pin which can make builds non-reproducible; update the workflow to pin
luacheck to a stable version (e.g., "1.2.0-1") by changing the install command
in the Lint job to install that specific version so all CI runs use the same
luacheck release.
🪄 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: 89138be6-d7e3-4869-8377-ff6262ed7144
📒 Files selected for processing (5)
.github/workflows/test.ymlapi.mdlib/resty/openapi_validator/init.luat/conformance/test_issue689.luat/conformance/test_validate_readonly.lua
✅ Files skipped from review due to trivial changes (1)
- api.md
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/resty/openapi_validator/init.lua
jarvis9443
added a commit
that referenced
this pull request
Apr 21, 2026
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
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.
Refactor all source code to follow lua-resty-etcd code style conventions.
Source Code Style
=signs, localize builtins at file toptab_insert,str_find,sub_str, etc.) instead of short namess:gsub(),s:gmatch()) with localized functions--- @param,--- @return), use simple--comments_VMT→_validator_mt,_MT→_router_mt{version = 0.1})readOnly→read_only,writeOnly→write_onlyInfrastructure
.luacheckrcwithstd = "ngx_lua"### target:patterninstall,dev,helptargetslua-cjsonto rockspec dependenciesDocumentation
api.mdwith detailed API documentationcoerce_types/fail_fastoptions)readOnly/writeOnlyandform-urlencodedto feature matrixAll existing tests pass. Zero lint warnings.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests